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

Add steiner tree functionality to rustworkx-core #1103

Merged
merged 14 commits into from
Mar 4, 2024

Conversation

mtreinish
Copy link
Member

This PR is the continuation of #916 that finishes the port of the steiner_tree() and metric_closure() functions to rustworkx-core to enable their reuse in rust. In particular this was especially tricky because the petgraph traits make it exceedingly difficult to have a generic function that takes in a graph for analysis and modifies it as most of the traits required for visiting/iteration that are used for analysis are only defined on borrowed graphs, and the limited traits for modifying graphs are defined on owned graph types. This causes a conflict where you can't easily express that a generic type G created in a function from a user input is both mutated using a trait and analyzed as there is a type mismatch between G and &G. After spending far too long and failing to find a pattern to express this, I opted to just use a discrete type for the return and leave the actual graph mutation up to the rustworkx-core user because we're lacking the ability to cleanly express what is needed via petgraph. Since this was a significant rewrite of #916 and only the core algorithm (which was ported from the original rustworkx code anyway) remained the same I opted to open up a separate PR built on top of #916's branch to avoid confusion.

Closes #916
Co-authored-by: Ryuhei Yoshida [email protected]

yoshida-ryuhei and others added 12 commits February 21, 2024 07:19
This commit finishes the rustworkx-core port of the steiner tree and
metric closure functions. In particular this was especially tricky
because the petgraph traits make it exceedingly difficult to have a
generic function that takes in a graph for analysis and modifies it as
most of the traits required for visiting/iteration that are used for
analysis are only defined on borrowed graphs, and the limited traits for
modifying graphs are defined on owned graph types. This causes a
conflict where you can't easily express that a generic type G created in
a function from a user input is both mutated using a trait and analyzed
as there is a type mismatch between G and &G. After spending far too
long to fail to find a pattern to express this, I opted to just use a
discrete type for the return and leave the actual graph mutation up to
the rustworkx-core user because we're lacking the ability to cleanly
express what is needed via petgraph.
@mtreinish mtreinish added this to the 0.15.0 milestone Feb 21, 2024
@coveralls
Copy link

coveralls commented Feb 21, 2024

Pull Request Test Coverage Report for Build 8132907756

Details

  • 396 of 410 (96.59%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.05%) to 96.434%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/steiner_tree.rs 56 60 93.33%
rustworkx-core/src/steiner_tree.rs 340 350 97.14%
Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_dijkstra.rs 3 96.35%
Totals Coverage Status
Change from base Build 8127117962: -0.05%
Covered Lines: 17089
Relevant Lines: 17721

💛 - Coveralls

@mtreinish mtreinish changed the title Steiner tree rustworkx core Add steiner tree functionality to rustworkx-core Feb 21, 2024
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.

The algorithm implementation looks the same and should be good, I left some comments about handling invalid inputs and errors though.

Thanks for resurrecting the PR!

tests/graph/test_steiner_tree.py Show resolved Hide resolved
src/steiner_tree.rs Outdated Show resolved Hide resolved
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.

Thanks! This should be good to go

@mtreinish mtreinish merged commit c8172d5 into Qiskit:main Mar 4, 2024
28 checks passed
@mtreinish mtreinish deleted the steiner-tree-rustworkx-core branch March 4, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants