-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor graph package (addresses #166) #170
Conversation
TODO: fix tests
This reverts commit 6738255.
I feel that this is entering bike-shedding a bit, yet this conversation is also crucial in terms of making the API more understandable.
Let's just pick one and move on; I don't want to use graph terminology solely because it confuses people with the data flow/dependency graph issue (since the class is called |
In real life, children depend on parents. Here, it is reversed. I never liked the dependency graph formulation. Having arrows that point in the opposite direction of data flow confuses me. |
1 similar comment
In real life, children depend on parents. Here, it is reversed. I never liked the dependency graph formulation. Having arrows that point in the opposite direction of data flow confuses me. |
The reason we use and keep using a dependency graph, imo, is to keep some algorithmic advantage (since we trace to upstream reactions more often) ChatGPT suggested |
Same for me. The refactoring that @axmmisaka is working on is meant to address this. |
I think it would be great if we could figure out a balanced plan during today's meeting, and execute it by today. We might be spending too much time on this and the issue is not very technical...... If we were to keep the underlying data structure to be a dependency graph, maybe aliasing would be the best idea to prevent headache...... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've arrived at a point to where this looks ready to merge...
As I add tests, I discovered that there are some small bugs; I'll fix them, then this should be ready to be merged...... |
I fixed a bug in the mermaid representation's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
This PR fixes #166.
addBackEdges
/addEdges
)toString
to usemermaid.js
representationhasCycle
; the one infromDependencyGraph
will incur some overhead so omitted)Reactor