Skip to content
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

Merged
merged 29 commits into from
Jun 28, 2023
Merged

Conversation

axmmisaka
Copy link
Collaborator

@axmmisaka axmmisaka commented Jun 22, 2023

This PR fixes #166.

  • Rename APIs so they less confusing
  • Remove unused or redundant APIs (removed addBackEdges/addEdges)
  • Move toString to use mermaid.js representation
  • Rewrite search functions to use iteration, and use more efficient algorithm (rewrote hasCycle; the one in fromDependencyGraph will incur some overhead so omitted)
  • Refactor improper use of graph APIs in Reactor

@axmmisaka axmmisaka marked this pull request as ready for review June 24, 2023 19:19
src/core/graph.ts Outdated Show resolved Hide resolved
src/core/graph.ts Outdated Show resolved Hide resolved
__tests__/dependencies.ts Outdated Show resolved Hide resolved
src/core/graph.ts Outdated Show resolved Hide resolved
src/core/graph.ts Outdated Show resolved Hide resolved
src/core/graph.ts Outdated Show resolved Hide resolved
@axmmisaka
Copy link
Collaborator Author

axmmisaka commented Jun 27, 2023

I feel that this is entering bike-shedding a bit, yet this conversation is also crucial in terms of making the API more understandable.

Original Data Flow/Precedence Graph Perspective Dependency Graph Perspective Alias
getEdges getUpstreamNeighbor getChildren getChildren + getUpstreamNeighbor
getBackEdges getDownstreamNeighbor getParents getParents + getDownstreamNeighbor
rootNodes sourceNodes/nodesWithNoOrigin? sinkNodes sourceDataNodes+sinkGraphNodes
leafNodes sinkNodes/nodesWithNoEffect? sourceNodes sinkDataNodes+sourceGraphNodes

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 DependencyGraph, it might be natural to assume no assumption on the data flow; although we use the graph solely to model the data flow between reactions/ports of reactors), but if that's the best way to do it I have no objections as long as they are clearly documented.

@edwardalee
Copy link
Collaborator

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
@edwardalee
Copy link
Collaborator

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.

@axmmisaka
Copy link
Collaborator Author

axmmisaka commented Jun 27, 2023

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)
I think the best way to go is to abstract out the existence of a dependency graph. We expose an abstract data model that represents reaction hierarchy without mentioning dependency or precedence, and name the APIs with natural data flow

ChatGPT suggested ReactionNetwork or ReactionGraph

@lhstrh
Copy link
Member

lhstrh commented Jun 27, 2023

I never liked the dependency graph formulation. Having arrows that point in the opposite direction of data flow confuses me.

Same for me. The refactoring that @axmmisaka is working on is meant to address this.

@axmmisaka
Copy link
Collaborator Author

I never liked the dependency graph formulation. Having arrows that point in the opposite direction of data flow confuses me.

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......

src/core/graph.ts Outdated Show resolved Hide resolved
src/core/reaction.ts Outdated Show resolved Hide resolved
src/core/reactor.ts Outdated Show resolved Hide resolved
Copy link
Member

@lhstrh lhstrh left a 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...

@axmmisaka
Copy link
Collaborator Author

As I add tests, I discovered that there are some small bugs; I'll fix them, then this should be ready to be merged......

@axmmisaka
Copy link
Collaborator Author

I fixed a bug in the mermaid representation's edgesWithIssue, added tests to test edgesWithIssue, and ran prettier on relevant files. I think this should be good to go, other than more comments on queue.ts which we could and should address later.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@lhstrh lhstrh merged commit b4e7ac4 into master Jun 28, 2023
1 of 2 checks passed
@lhstrh lhstrh deleted the axmmisaka/refactor-graph branch June 28, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graph Issues
3 participants