-
Notifications
You must be signed in to change notification settings - Fork 145
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
Implementation of cycle_basis_edges #885
base: main
Are you sure you want to change the base?
Conversation
|
Pull Request Test Coverage Report for Build 9962098330Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So overall this looks like a good start. The big things that I think are missing are python side tests to validate the python api is working as expected and also a release note (https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#release-notes).
The other small concern I have is the amount of duplication between cycle_basis_edges
and cycle_basis
. The code is mostly the same between the function and I'm thinking it would be good to combine them into a single code path.
As for the edge weight for the python side you can call weight.clone_ref(py)
as it's a PyObject
type and we can clone it with a GIL handle (which is what the Python
type gives us).
/// let graph = UnGraph::<i32, i32>::from_edges(&edge_list); | ||
/// let mut res: Vec<Vec<(NodeIndex, NodeIndex)>> = cycle_basis_edges(&graph, Some(NodeIndex::new(0))); | ||
/// ``` | ||
pub fn cycle_basis_edges<G>(graph: G, root: Option<G::NodeId>) -> Vec<Vec<(G::NodeId, G::NodeId)>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of returning Vec<(G::NodeId, G::NodeId)>
here it might be better to return Vec<G::EdgeId>
. Then if we want to return the edge endpoint or weight we can just look it up from the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's funny you mention this, as in an earlier version, I had the function return the EdgeId
attribute of each EdgeRef
but replaced it with the tuple thinking that it should instead return the edge objects.
Also, I'm assuming that by returning the edge endpoint you mean returning it using the wrapper located in rustworkx/src/connectivity/mod.rs
lines 100 to 110. Should I use the graph.edges() method fetch the edges by index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By edge endpoints I meant what you're doing now. For each edge you find you're mapping to (edge.source(), edge.target())
(the endpoints being the tuple of nodes for the edge), but instead if you did edge.id()
that would give you the unique G::EdgeId
for the edge. Then if you returned a Vec<G::EdgeId>
callers of this function could look up any attributes of the edge they needed from that list. For examples if you wanted a list of endpoint tuples you could do something like:
let edge_endpoints = cycle_basis(graph, None)
.into_iter()
.filter_map(|e| graph.edge_endpoints(e))
.collect()
but it would also give you the flexibility to get the weights, or other details. It basically is just returning a reference to the edge in graph
instead of one of properties of the edge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I will make those changes accordingly.
This looks pretty good to me, just a quick question: if there a reason to declare both a |
Well, there might be a better way of doing so that doesn't involve two vectors. The reason I did it this way is that I couldn't conditionally assign this variable with different types ( I could, however, change the enum so that it represents the individual nodes/edges (instead of The problem happens when importing this enum into the wrapper function, which complains about the types in use. I could try and get this to work with enough time. |
I mean if it works fine, I don't see any reason to change it, I was just asking. Maybe ask @mtreinish. Otherwise, looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast update. This is look better from a code duplication standpoint and is making good progress. I left some inline comments mainly about the interface split. I have some concerns that the current version of the code has some breaking API changes to rustworkx-core that we can't make without potentially impacting users. I left some inline suggestions on how we can tweak this a bit to avoid that kind of impact.
impl<N, E> EdgesOrNodes<N, E> { | ||
pub fn unwrap_nodes(self) -> Vec<Vec<N>> { | ||
use EdgesOrNodes::*; | ||
match self { | ||
Nodes(x) => x, | ||
Edges(_) => vec![], | ||
} | ||
} | ||
pub fn unwrap_edges(self) -> Vec<Vec<E>> { | ||
use EdgesOrNodes::*; | ||
match self { | ||
Edges(x) => x, | ||
Nodes(_) => vec![], | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these methods used for anything outside of the tests? If not I don't think we should add them to the public interface since they're a bit custom purpose to simplify testing and I think they'd be better served as a function in the tests (although based on my earlier comments about the interface split I don't think we should expose EdgesOrNodes
publicly and if not I think we can just handle all of this internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since we're using a custom return type that can't be interpreted natively as Vec<Vec<NodeId>>
or Vec<Vec<EdgeId>>
the values need to be unwrapped.
Although this could technically be done inside of the public-facing functions that were part of your review, we'd have to rewrite the same procedure twice over (in testing and in the public-facing functions, which is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, thanks for the fast updates on this, I have a few more comments inline.
fixes: | ||
- | | ||
Support for edges when getting the cycle basis. Refer to | ||
`#551 <https://github.com/Qiskit/rustworkx/issues/551>`__ for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't actually need this release note what you have for the feature note is more than sufficient to document the new feature for this release. This is really just a new feature there isn't any bug being fixed here. The github "issues" tracker is really both for tracking bugs and also for feature requests.
- | | ||
A function, :func:`~rustworkx.cycle_basis_edges` was added to the crate | ||
``rustworkx-core`` in the ``connectivity`` module. This function returns | ||
the edge indices that form the cycle basis of a graph. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to split this into 2 release notes, one for the rustworkx-core function and one for the python side function being added.
- | | |
A function, :func:`~rustworkx.cycle_basis_edges` was added to the crate | |
``rustworkx-core`` in the ``connectivity`` module. This function returns | |
the edge indices that form the cycle basis of a graph. | |
- | | |
A function, ``cycle_basis_edges`` was added to the crate | |
``rustworkx-core`` in the ``connectivity`` module. This function returns | |
the edge indices that form the cycle basis of a graph. | |
- | | |
Added a new function :func:`~rustworkx.cycle_basis_edges` which is similar | |
to the existing :func:`~.cycle_basis` function but instead of returning node | |
indices it returns a list of edge indices for the cycle. |
- Inspired from the existing procedure `cycle_basis`. - Modified to return a tuple of `NodeId` in format `(source, target)`. - Added API calls so that method works in Python.
- Cycle_basis_edges now returns a list of edgeIDs.
- Added `TestCycleBasisEdges` class for unit testing. - Added similar tests to those present on `TestCycleBasis`. - Tests are similar due to the same methodology. - Graphs used for testing are different.
- Modified get_edge_between to only check target when equiv == false.
- Cycle basis edges is now within the codebase of cycle basis. - The `EdgesOrNodes` enum handles the different return data types.
- Release notes now include an example with a jupyter notebook.
- A previous commit inadvertedly made cycle_basis prublic. - To address this, two new public-facing functions were added: - `cycle_basis` for NodeId returns. - `cycle_basis_edges` for EdgeId returns. - When customizing cycle_basis a new enum EdgesOrNodes was made. - Enum should not be have been shared by definition. - Correction makes it private. - Removed redundant import from unwrap methods. - Tests have been adjusted accordingly.
- The method returns the first edge found between nodes. - The docstring for this method was corrected.
f531223
to
075b2f0
Compare
- Use while let statement to continuously pop the queue.
Aims to fix #551
cycle_basis
.NodeId
in the format(source, target)
.weight()
method from EdgeRef returns a reference and cannot be copied over.