Move dag longest path functions to rustworkx-core. #1192
Merged
+280
−52
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Started to port `longest_path` to rustworkx by first adding `longest_path.rs` to `rustworkx-core/src/lib/rs`. Differences from the original in `rustworkx/src/dag_algo/longest_path.rs`: - Removed Python-specific code: The function no longer uses pyo3 and PyObject. Instead, it uses generic Rust types. - Type flexibility: The function parameters now accept any type N for nodes, E for edges, and a weight function F that maps edges (represented by their node indices and edge reference) to a weight of type T. This allows the function to work with different graph and edge types. - Error handling: Instead of returning a Result, it returns an Option to signify failure only in the case of a cycle, which is preemptively checked at the beginning. - Using Ord and Add: These trait bounds on T allow for ordering and arithmetic, necessary for determining the longest path.
Explanation of Test Cases: - test_empty_graph: Checks that an empty graph returns a path of length zero with no nodes. - test_single_node: Checks that a graph with a single node returns the node itself with zero path length (since there's no edge). - test_simple_path: A simple two-node graph with one edge. Tests if the function correctly identifies the longest path. - test_dag_with_multiple_paths: A Directed Acyclic Graph (DAG) with three nodes and a choice of paths, to test if the function picks the longest one. - test_graph_with_cycle: Ensures that the function returns None for a graph with a cycle, as longest path calculations should only be done on DAGs. These test can be run with `cargo test` or with `cargo test test_longest_path`
Added function descriptions and comments to `longest_path.rs`. The function description is based off of the format in `rustworkx-core/src/line_graph`.
…50/rustworkx into 1168-move-dag-functions
Pull Request Test Coverage Report for Build 9102860038Details
💛 - Coveralls |
Slight optimizations for `longest_path.rs`
Adjusted the function to accept `G` instead of a `DiGraph` where `G` has the mentioned traits to represent any directed graph. This allows the function to be more general and apply to both `DiGraph` and `StableDiGraph`. I also changed some other parts of the code to match the original for consistency. However, there is a current issue with compiling the tests, as for some reason, the tests do not satisfy the trait `EdgeRef`.
Adjusted parameters to that F now represents a function that will pass the `EdgeRef` for the edges in the graph. Before we had an error when passing the `EdgeRef` as a trait of `G`, but it should actually be used for `F`. Adjusted the tests to account for that change and added additional tests of StableDiGraph and negative weights.
Converted the return type from ``Some((Vec<NodeIndex>, T))`` to `Some((Vec<usize>, T))` to match the original function more. May need to change back to `NodeIndex` if it makes more sense to use it, as I can see a couple of cases where that is more ideal.
Changed `longest_path` to call `core_longest_path` (the implementation in rustworkx_core) by creating the `dag` and `edge_cost` parameters for that function. However, there is an issue of unvalid weights, which I am unsure how to handle at the moment. All tests seem to pass except for `digraph.test_depth.TestWeightedLongestPath.test_nan_not_valid_weight`.
henryzou50
commented
May 14, 2024
Changed the return type to handle the kind of errors that the weight function can return. Made changes so that it is more similar to how we implemented `dijkstra`. Added a small test case for this change.
Updated the edge_cost to return `Result<T, PyErr>` and updated error handling. Now all tests pass. Also updated style and added an example.
Summary of Changes: - Change variable names for clarity: `incoming_path` instead of `us` and `max_path` instead of `maxu` - Simplified while Loop Condition: Replaced complex match statement with map_or for clearer and more concise logic in the backtracking loop. - Optimized HashMap Initialization: Pre-allocated HashMap with capacity equal to the number of nodes to improve performance and memory usage. - Enhanced Maximum Distance Calculation: Used into_iter and unwrap_or to simplify logic and improve readability by directly handling maximum value computation and default cases.
Added a function description to `longest_path` in `src/dag_algo/longest_path.rs`
henryzou50
changed the title
mtreinish
reviewed
May 15, 2024
There was a problem hiding this comment.
Overall this looks great, thanks for doing this @henryzou50! I left a couple inline comments the only two things are really the return type for the output Vec when a path is found and the new module's name. Otherwise I think this is good to go.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Rename `longest_path.rs` to `dag_algo` as the `longest_path` function works only for DAGs and now we can include other DAG function here.
Changed the return type from `Some((Vec<usize>, T))` to `Some((Vec<NodeId<G>>, T))`, as it makes it easier for users to use `G::NodeId` (which is the generic form of NodeIndex). Also added aliases for readbility.
|
@mtreinish thanks for the review and comments! I applied the changes you suggested and it should be ready to go now. |
mtreinish
approved these changes
May 15, 2024
mtreinish
added a commit
to mtreinish/retworkx
that referenced
this pull request
May 17, 2024
In the recently merged Qiskit#1192 a new generic DAG longest_path function was added to rustworkx-core. However, the trait bounds on the function were a bit tighter than they needed to be. The traits were forcing NodeId to be of a NodeIndex type and this wasn't really required. The only requirement that the NodeId type can be put on a hashmap and do a partial compare (that implements Hash, Eq, and PartialOrd). Also the IntoNeighborsDirected wasn't required because it's methods weren't ever used. This commit loosens the traits bounds to facilitate this. At the same time this also simplifies the code structure a bit to reduce the separation of the rust code structure in the rustworkx crate using longest_path().
IvanIsCoding
pushed a commit
that referenced
this pull request
May 18, 2024
In the recently merged #1192 a new generic DAG longest_path function was added to rustworkx-core. However, the trait bounds on the function were a bit tighter than they needed to be. The traits were forcing NodeId to be of a NodeIndex type and this wasn't really required. The only requirement that the NodeId type can be put on a hashmap and do a partial compare (that implements Hash, Eq, and PartialOrd). Also the IntoNeighborsDirected wasn't required because it's methods weren't ever used. This commit loosens the traits bounds to facilitate this. At the same time this also simplifies the code structure a bit to reduce the separation of the rust code structure in the rustworkx crate using longest_path().
SILIZ4
pushed a commit
to SILIZ4/rustworkx
that referenced
this pull request
May 18, 2024
…kit#1195) In the recently merged Qiskit#1192 a new generic DAG longest_path function was added to rustworkx-core. However, the trait bounds on the function were a bit tighter than they needed to be. The traits were forcing NodeId to be of a NodeIndex type and this wasn't really required. The only requirement that the NodeId type can be put on a hashmap and do a partial compare (that implements Hash, Eq, and PartialOrd). Also the IntoNeighborsDirected wasn't required because it's methods weren't ever used. This commit loosens the traits bounds to facilitate this. At the same time this also simplifies the code structure a bit to reduce the separation of the rust code structure in the rustworkx crate using longest_path().
IvanIsCoding
added a commit
that referenced
this pull request
May 22, 2024
* add hyperbolic random graph model generator * Loosen trait constraints and simplify structure for longest_path (#1195) In the recently merged #1192 a new generic DAG longest_path function was added to rustworkx-core. However, the trait bounds on the function were a bit tighter than they needed to be. The traits were forcing NodeId to be of a NodeIndex type and this wasn't really required. The only requirement that the NodeId type can be put on a hashmap and do a partial compare (that implements Hash, Eq, and PartialOrd). Also the IntoNeighborsDirected wasn't required because it's methods weren't ever used. This commit loosens the traits bounds to facilitate this. At the same time this also simplifies the code structure a bit to reduce the separation of the rust code structure in the rustworkx crate using longest_path(). * use vector references * change to slice (clippy) * generalize to H^D, improve numerical accuracy * allow infinite coordinate * handle infinity in hyperbolic distance * remove unused import (clippy) * fix python stub --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attempt to Solve #1168
Summary:
We are moving the
longest_pathfunction torustworkx-core. The main implementation will be added torustworkx-core/src/longest_path.rs, utilizing only Rust generics. The existingsrc/dag_algo/longest_path.rswill be updated to serve as the Python interface. Since we are retaining this file, updates tosrc/dag_algo/mod.rs, where the four DAG longest path functions reside, are not necessary.Details
longest_path.rstorustworkx-core/src/rustworkx-core/src/longest_path.rssrc/dag_algo/longest_path.rsto utilizerustworkx-core/src/longest_path.rs