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

Move collect_bicolor_runs() to rustworkx-core #1186

Merged
merged 18 commits into from
Jun 10, 2024

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented May 8, 2024

This is a first attempt at solving #1166, the PR introduces a new collect_bicolor_runs() function in rustworx-core and modifies the python-facing collect_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.

graph: G,
filter_fn: F,
color_fn: C,
) -> Result<Vec<Vec<&<G as Data>::NodeWeight>>, CollectBicolorSimpleError<E>> //OG type: PyResult<Vec<Vec<PyObject>>>
Copy link
Member

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

rustworkx-core/src/lib.rs Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented May 8, 2024

Pull Request Test Coverage Report for Build 9446628745

Details

  • 96 of 97 (98.97%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.004%) to 95.837%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rustworkx-core/src/dag_algo.rs 74 75 98.67%
Files with Coverage Reduction New Missed Lines %
rustworkx-core/src/generators/random_graph.rs 2 85.04%
Totals Coverage Status
Change from base Build 9429147177: -0.004%
Covered Lines: 17380
Relevant Lines: 18135

💛 - Coveralls

@ElePT ElePT changed the title [WIP] Move collect_bicolor_runs() to rustworkx-core Move collect_bicolor_runs() to rustworkx-core May 29, 2024
@ElePT ElePT marked this pull request as ready for review May 29, 2024 16:15
rustworkx-core/src/dag_algo.rs Outdated Show resolved Hide resolved
rustworkx-core/src/dag_algo.rs Show resolved Hide resolved
@mtreinish mtreinish added the rustworkx-core Issues tracking adding functionality to rustworkx-core label Jun 2, 2024
Copy link
Member

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

rustworkx-core/src/dag_algo.rs Outdated Show resolved Hide resolved
rustworkx-core/src/dag_algo.rs Show resolved Hide resolved
rustworkx-core/src/dag_algo.rs Outdated Show resolved Hide resolved
rustworkx-core/src/dag_algo.rs Outdated Show resolved Hide resolved
rustworkx-core/src/dag_algo.rs Show resolved Hide resolved
ElePT and others added 3 commits June 6, 2024 13:09
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ElePT
Copy link
Contributor Author

ElePT commented Jun 6, 2024

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.

@ElePT ElePT requested a review from mtreinish June 7, 2024 07:57
Copy link
Member

@mtreinish mtreinish left a 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 Show resolved Hide resolved
rustworkx-core/src/dag_algo.rs Show resolved Hide resolved
rustworkx-core/src/dag_algo.rs Show resolved Hide resolved
rustworkx-core/src/dag_algo.rs Show resolved Hide resolved
G: IntoNodeIdentifiers // Used in toposort
+ IntoNeighborsDirected // Used in toposort
+ IntoEdgesDirected // Used for .edges_directed
+ IntoEdgeReferences
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

ElePT and others added 3 commits June 10, 2024 11:44
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@mtreinish mtreinish added the automerge Queue a approved PR for merging label Jun 10, 2024
@mtreinish mtreinish merged commit c7a7d53 into Qiskit:main Jun 10, 2024
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging rustworkx-core Issues tracking adding functionality to rustworkx-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants