-
Notifications
You must be signed in to change notification settings - Fork 145
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
Add reverse
argument to topological sorters
#1108
Conversation
This allows `lexicographical_topological_sort` and `TopologicalSorter` to produce a topological sort that would have been found if the edges in the graph had reversed directions, without constructing the edge-flipped graph. This is useful for graphs where we might want to do topological analysis from both ends of the data flow, for example in Qiskit where we occasionally want to find the "final" operations topologically.
Pull Request Test Coverage Report for Build 8116939754Details
💛 - Coveralls |
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.
LGTM, the implementation is very clever and I like the small dif.
My only comment is for encapsulating the logic of the directions to be overprotective. Especially given we revisit topological sorting once in a while
src/dag_algo/mod.rs
Outdated
let (in_dir, out_dir) = if reverse { | ||
(petgraph::Direction::Outgoing, petgraph::Direction::Incoming) | ||
} else { | ||
(petgraph::Direction::Incoming, petgraph::Direction::Outgoing) | ||
}; |
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.
Can you create a public function in dag_algo
to encapsulate this logic and reuse it in the other files? It is a nitpick but just to avoid the kind of bugs where we flip it
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.
Done in 12a91ec.
Sorry for disappearing for a while there. I've added the helper in 12a91ec. |
This allows
lexicographical_topological_sort
andTopologicalSorter
to produce a topological sort that would have been found if the edges in the graph had reversed directions, without constructing the edge-flipped graph.This is useful for graphs where we might want to do topological analysis from both ends of the data flow, for example in Qiskit where we occasionally want to find the "final" operations topologically.
Close #1105