-
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
Move collect_bicolor_runs()
to rustworkx-core
#1186
Conversation
graph: G, | ||
filter_fn: F, | ||
color_fn: C, | ||
) -> Result<Vec<Vec<&<G as Data>::NodeWeight>>, CollectBicolorSimpleError<E>> //OG type: PyResult<Vec<Vec<PyObject>>> |
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.
Instead of returning NodeWeight
here I think it would be simpler to just return something like:
impl Iterator<Item=Vec<G::NodeId>>
(or something like that I might have made a mistake in the exact syntax) Or alternatively a Vec<Vec<G::NodeId>>
The idea is to return an iterator of node indices for each run. This isn't exactly what the python api was returning, but retunring the node ids will let us easily iterate and map that to the weights for python, but for other rust consumer (especially Qiskit in the future) this will be lighter weight to work with because it's a simple vec get to go from an index to a weight.
Then using an iterator would be a more rust native interface to return an iterator of the runs
Pull Request Test Coverage Report for Build 9446628745Details
💛 - Coveralls |
collect_bicolor_runs()
to rustworkx-corecollect_bicolor_runs()
to rustworkx-core
…d unit tests and reno.
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 and I think it's ready to merge except for the missing assertion in the docstring example. There is an open question which I'm fine either way on which is whether the filter function and coloring functions are passed node weights or node ids, I can go either way so I'm curious your thoughts on it.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
…s in tests for clarity.
I have tried out the suggestion of using ids instead of weights in c256e27, the only impact I noticed on the user side was the need to switch from one shared function for all tests to per-test closures in the unit tests, because a reference to the graph is needed to get the weights from the node/edge ids. I actually think this makes the tests more readable. I had hoped to be able to get rid of the result annotations, but the error type still needs to be determined, shame. |
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 now thanks for the updates. I just left some whitespace nits in the docstring. Also I think we can drop one trait bound.
rustworkx-core/src/dag_algo.rs
Outdated
G: IntoNodeIdentifiers // Used in toposort | ||
+ IntoNeighborsDirected // Used in toposort | ||
+ IntoEdgesDirected // Used for .edges_directed | ||
+ IntoEdgeReferences |
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.
Is this used anywhere?
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
This is a first attempt at solving #1166, the PR introduces a new
collect_bicolor_runs()
function inrustworx-core
and modifies the python-facingcollect_bicolor_runs
to use the core function under the hood (no user-facing changes).The current function returns a
<Vec<Vec<G::NodeId>>
, I could not find a way to implement the iterator mentioned in the original issue because I kept getting "the size cannot be known at compilation time", but I am open to implementation suggestions.