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.
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 |
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
initialnodes 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
initialis chosen such that only nodes dominated by theinitialset will be returned. In other words, if a node has zero in degree, but is not in theinitialset, 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
TopologicalSorters in a loop while adding nodes to one particular graph, and being able to pass theinitialnodes lets us avoid a quadratic complexity in doing so.