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 graph module, port contract_nodes and has_parallel_edges to core. #1143

Merged
merged 13 commits into from
May 22, 2024

Conversation

kevinhartman
Copy link
Contributor

@kevinhartman kevinhartman commented Mar 15, 2024

Important

In the original PR description, I'd suggested we use blanket implementations for all graph types, constrained to apply to compatible graph types using traits. However, this proved not to be ideal, since petgraph does not provide a trait to identify stable versus unstable graphs, yet most algorithms would need to rely on this property to be written efficiently.

Instead, the PR now recommends that we provide explicit implementations per petgraph graph type for ported algorithms.

As part of #1121, this PR is meant as a proposal for a few patterns we might follow to bring rustworkx-core up to parity with our Python API.

To demonstrate these patterns, I've ported contract_nodes and has_parallel_edges from PyGraph and PyDiGraph to rustworkx-core, making the functions available for all stable petgraph graphs (chosen as "complex" and "simple" examples, respectively).

Note

Many of the methods that currently exist on Py(Di|)Graph are there only to make the Python API more ergonomic (i.e. to side-step PyO3 limitations with iterators and data ownership). These methods should probably not be ported to core. In general, rustworkx-core's API should feel like a Rust API.

Extension traits

The primary pattern proposed here is to provide traits for the various graph methods of Py(Di|)Graph worth porting to core, along with implementations for applicable petgraph graph types.

To aid in discoverability, we can also expose modules intended for import via * that bring all supported extension methods into scope for the graph type. For example, we can import all graph extensions as follows (which, at least in the CLion IDE, adds per-graph compatible methods to code completion):

use petgraph::prelude::*;

// Imports all extension methods, including `contract_nodes`.
use rustworkx_core::graph_ext::*;

let mut dag: StableDiGraph<char, usize> = ...;
let res = dag.contract_nodes(...);

Node contraction

To see how this idea works with a complex example, I've ported contract_nodes from Py(Di|)Graph. To do this, we introduce 4 different traits: ContractNodesUndirected, ContractNodesSimpleUndirected, ContractNodesDirected, and ContractNodesSimpleUndirected.

We need this many because each of the methods between ContractNodes(Un|Di)rected and ContractNodesSimple(Un|Di)rected has a different signature and return type (since each has a different set of possible errors).

Error interface

In #1124 we started discussing how the core's error interface ought to look. Originally, I proposed that we should aim to have a single error type returned by all core functions which may fail. However, I see now that this would have been somewhat of a Rust anti-pattern: it's generally encouraged to have error types which can indicate multiple failure cases, but it's not proper for functions to return error types with variants that can never happen in the given function's execution.

Instead, we define unique error enum types for each function to encapsulate exactly the error set it may return. For example, we introduce two error types ContractError and ContractSimpleError for node contraction, since only simple contractions can fail due to an edge merge issue.

For errors, I'd propose putting all error types within src/err.rs. These can then be imported internally by extension trait implementations in the graph module, as well as by top-level functions outside it.

On the Python-side, we can implement From<...> for RxPyErr to automatically map each error type to a Python exception. This is done for both ContractError and ContractSimpleError. See the docstring for RxPyErr for more detail.

Integration testing

I've added a new folder tests/graph_ext which contains a main.rs file. Any modules registered there get automatically picked up and included in the same integration testing executable. For now, there's a file contraction.rs which contains modules for testing against StableGraph and GraphMap (the two stable graph types in petgraph). Additional algorithms ported to graph_ext should be added to this folder.

Other

  • I've also added a new trait NodeRemovable, which we've been needing for a while. It's similar to the Build trait provided by petgraph, but provides node removal.
  • In case it isn't clear, the graph module has been added to house our extension methods. Top-level functions can still make sense, and I don't propose we move existing ones. But perhaps for some, we ought to consider exposing them as extensions in parallel for user convenience.

Todo

  • Port Python tests for contract_nodes and has_parallel_edges to core.

mtreinish added a commit to mtreinish/retworkx that referenced this pull request Apr 1, 2024
This commit bumps the MSRV for rustworkx in 0.15.0 to 1.70.0. This
enables us to use GATs (see Qiskit#1143) and also enables to update some of
our dependencies which require newer versions of rust to compile.
@mtreinish mtreinish mentioned this pull request Apr 1, 2024
mergify bot pushed a commit that referenced this pull request Apr 2, 2024
* Bump MSRV to 1.70

This commit bumps the MSRV for rustworkx in 0.15.0 to 1.70.0. This
enables us to use GATs (see #1143) and also enables to update some of
our dependencies which require newer versions of rust to compile.

* Use 1.70 instead of 1.7

* Update .github/workflows/main.yml

---------

Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
@mtreinish mtreinish added this to the 0.15.0 milestone Apr 4, 2024
@coveralls
Copy link

coveralls commented Apr 11, 2024

Pull Request Test Coverage Report for Build 9190954784

Details

  • 243 of 339 (71.68%) changed or added relevant lines in 7 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.5%) to 96.001%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib.rs 13 22 59.09%
rustworkx-core/src/graph_ext/mod.rs 3 16 18.75%
rustworkx-core/src/err.rs 0 16 0.0%
rustworkx-core/src/graph_ext/contraction.rs 177 235 75.32%
Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_bellman_ford.rs 6 95.53%
Totals Coverage Status
Change from base Build 9190944049: -0.5%
Covered Lines: 16973
Relevant Lines: 17680

💛 - Coveralls

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.

Overall this LGTM. I'm happy with a lot of the patterns you're introducing here as I feel like they're easily extendable for additional use cases. I just had a couple of small inline questions and comments.

The other though was maybe we should add a prelude so that users can do use rustworkx_core::prelude::* and get all the extension traits and error structs from a single place. I think we should probably leave the algorithm functions as standalone things, but putting the basics for interacting with the library in a prelude seems like the normal rust pattern.

rustworkx-core/src/graph_ext/mod.rs Show resolved Hide resolved
Comment on lines 58 to 66
#[allow(dead_code)]
fn fmt_node_not_found<N: Debug>(f: &mut Formatter<'_>, node: &N) -> std::fmt::Result {
write!(f, "Node not found in graph: {:?}", node)
}

#[allow(dead_code)]
fn fmt_edge_not_found<E: Debug>(f: &mut Formatter<'_>, edge: &E) -> std::fmt::Result {
write!(f, "Edge not found in graph: {:?}", edge)
}
Copy link
Member

Choose a reason for hiding this comment

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

If these aren't used is there a reason to keep them around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're here mostly because I'm hoping to establish a pattern, and I'm not confident there's enough here otherwise for it to stick.

I can remove if it's your preference, let me know!

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd rather remove the dead code or leave it as a comment or something. To me it's more confusing to leave this around I didn't see the pattern. I'm thinking if you want to make this a pattern I'd put this as a comment at the top using these as an example and explaining how they should be used.

rustworkx-core/src/graph_ext/contraction.rs Outdated Show resolved Hide resolved
@kevinhartman
Copy link
Contributor Author

kevinhartman commented May 9, 2024

This should be ready for another look. Thanks for the review @mtreinish!

EDIT: On the topic of a prelude, I think that's a good idea, but not even petgraph pulls in their visit stuff via their prelude, so I think we ought to think that through separately from this PR.

@mtreinish
Copy link
Member

EDIT: On the topic of a prelude, I think that's a good idea, but not even petgraph pulls in their visit stuff via their prelude, so I think we ought to think that through separately from this PR.

Sure, I agree. We can think about a rustworkx core prelude in a follow up for 0.16.0. I'm not sure exactly what we'd want to put it in it.

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, but it needs a release note (unless I missed it) to document the additions to rustworkx-core.

@mtreinish mtreinish added the automerge Queue a approved PR for merging label May 22, 2024
@mtreinish mtreinish merged commit 955d989 into Qiskit:main May 22, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants