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

Migrate Johnson's algorithm to rustworkx-core #1318

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

mtreinish
Copy link
Member

This commit adds Johnson's algorithm for computing simple cycles to rustworkx-core. This implementation returns an iterator-like struct which requires a reference to the graph for each step of the iterator. This mirrors the structure of the Bfs and similar structs in petgraph. While an implementor of Iterator could be used for this, this complicates using it from Python because we can't have a shared reference in a pyclass struct. This also has the advantage of enabling mutable references while iterating over the cycles.

To facilitate this 2 traits are added EdgeFindable and EdgeRemovable which add traits for remove_edge() and find_edge() to petgraph graph types. The EdgeRemovable trait isn't actually used currently, it was in an earlier local draft of the PR which attempted to remove self cycles from a clone before changing direction of the interface. Since the implementation was done and relatively simple, and the potential general usefulness this was left in. EdgeFindable is actively used by this new function however.

This commit adds Johnson's algorithm for computing simple cycles to
rustworkx-core. This implementation returns an iterator-like struct
which requires a reference to the graph for each step of the iterator.
This mirrors the structure of the Bfs and similar structs in petgraph.
While an implementor of Iterator could be used for this, this
complicates using it from Python because we can't have a shared
reference in a pyclass struct. This also has the advantage of enabling
mutable references while iterating over the cycles.

To facilitate this 2 traits are added `EdgeFindable` and `EdgeRemovable`
which add traits for `remove_edge()` and `find_edge()` to petgraph graph
types. The `EdgeRemovable` trait isn't actually used currently, it was
in an earlier local draft of the PR which attempted to remove self
cycles from a clone before changing direction of the interface. Since
the implementation was done and relatively simple, and the potential
general usefulness this was left in. `EdgeFindable` is actively used
by this new function however.
@mtreinish mtreinish added the rustworkx-core Issues tracking adding functionality to rustworkx-core label Nov 17, 2024
@coveralls
Copy link

coveralls commented Nov 17, 2024

Pull Request Test Coverage Report for Build 12365623964

Details

  • 288 of 312 (92.31%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 95.825%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rustworkx-core/src/connectivity/johnson_simple_cycles.rs 251 263 95.44%
rustworkx-core/src/graph_ext/mod.rs 0 12 0.0%
Totals Coverage Status
Change from base Build 12364370810: -0.04%
Covered Lines: 18360
Relevant Lines: 19160

💛 - Coveralls

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

LGTM but I left a comment about possibly keeping the old name of the struct given the return type is public. Of course most users just iterate over it but it would be nice not to change anything

src/connectivity/johnson_simple_cycles.rs Show resolved Hide resolved
@IvanIsCoding IvanIsCoding added this to the 0.16.0 milestone Nov 20, 2024
@mtreinish mtreinish added this pull request to the merge queue Dec 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
@IvanIsCoding
Copy link
Collaborator

I will try to clone the branch and fix.

I think the new rustdoc lint added recently complains about the docstring

@IvanIsCoding IvanIsCoding added this pull request to the merge queue Dec 17, 2024
Merged via the queue into Qiskit:main with commit 14be08f Dec 17, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rustworkx-core Issues tracking adding functionality to rustworkx-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants