-
Notifications
You must be signed in to change notification settings - Fork 147
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 initial
argument to topological sorters
#1128
Conversation
This adds a way for the topological sorters to recieve a set of initial nodes to begin the search from. This can allow the topological iteration to begin from a mid-point of the order, or to simply provide Rustworkx with the already-known set of initial nodes, skipping the iteration over each node to find the full set. Especially in `TopologicalSorter`, where one may be using the on-line search to find only the first handful of nodes in the topological order in an inner loop of a mutating graph, this can be a complexity improvement. The `initial` nodes are required to have no topological ordering between themselves, and it is an error if they do. There are other possible conventions to choose here, but this one is the easiest to state / understand and the fastest to execute; most other sensible conventions would require some amount of initial walking of the DAG to determine the partial topological order between the nodes before beginning, which would get in the way of the complexity improvements for inner-loop topological sorts. For similar complexity reasons, the behaviour surrounding zero-in-degree nodes that are _not_ in `initial` is chosen such that only nodes dominated by the `initial` set will be returned. In other words, if a node has zero in degree, but is not in the `initial` set, it won't be returned as part of the order. This is easily understandable and documentable ("the returned topological order is of all nodes dominated by `initial`"), and removes the need for the all-node iteration to locate any potential zero-in-degree nodes.
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 looks great. I liked the way you were able to detect the partial topological order between nodes using different methods for lexicographical_topological_sort
and TopologicalSorter
I noted some possible readability changes. There's also a typo in line 188 in toposort.py (not yours) 'to marks as done' s/b 'to mark as done' .
All the tests work fine. I was wondering if some additional random testing with larger graphs might uncover some issues these short graphs would not?
Co-authored-by: Edwin Navarro <enavarro@comcast.net>
I've pushed up the docs fixes and an update to make it PyO3-0.21 compatible. What sorts of randomised tests were you thinking of? I was having a little bit of a hard time imagining a randomised test that I could a) write (making a randomised DAG that actually samples the space of potential node orders well seemed like a non-trivial problem) and b) verify cleanly in a test seems not super easy to me. If you've got any ideas, I'm happy to expand the tests. |
Pull Request Test Coverage Report for Build 8571151224Details
💛 - 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.
This LGTM. I didn't have any specific random tests in mind. Just thinking there might be paths unexplored, but your tests cover any paths I can think of. Thanks,
This adds a way for the topological sorters to recieve a set of initial nodes to begin the search from. This can allow the topological iteration to begin from a mid-point of the order, or to simply provide Rustworkx with the already-known set of initial nodes, skipping the iteration over each node to find the full set. Especially in
TopologicalSorter
, where one may be using the on-line search to find only the first handful of nodes in the topological order in an inner loop of a mutating graph, this can be a complexity improvement.The
initial
nodes are required to have no topological ordering between themselves, and it is an error if they do. There are other possible conventions to choose here, but this one is the easiest to state / understand and the fastest to execute; most other sensible conventions would require some amount of initial walking of the DAG to determine the partial topological order between the nodes before beginning, which would get in the way of the complexity improvements for inner-loop topological sorts.For similar complexity reasons, the behaviour surrounding zero-in-degree nodes that are not in
initial
is chosen such that only nodes dominated by theinitial
set will be returned. In other words, if a node has zero in degree, but is not in theinitial
set, it won't be returned as part of the order. This is easily understandable and documentable ("the returned topological order is of all nodes dominated byinitial
"), and removes the need for the all-node iteration to locate any potential zero-in-degree nodes.Close #1106 - the motivation in there is that we end up building and using many
TopologicalSorter
s in a loop while adding nodes to one particular graph, and being able to pass theinitial
nodes lets us avoid a quadratic complexity in doing so.