-
Notifications
You must be signed in to change notification settings - Fork 195
Implementation of cycle_basis_edges #885
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
Conversation
CLA assistant check
All committers have signed the CLA.
CLAassistant
commented
May 31, 2023
CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
Results from testing in Python:
Pull Request Test Coverage Report for Build 14372847447Details
💛 - 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).
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 cycles_edges and a cycles_nodes if from what I understand we only return one or the other? Is it possible in Rust to declare variables like:
if want_edge: let mut cycles: Vec<Vec<G::EdgeId>> = Vec::new(); else: let mut cycles: Vec<Vec<G::NodeId>> = Vec::new();. This is low-key just for me to learn
if there a reason to declare both a cycles_edges and a cycles_nodes if from what I understand we only return one or the other? Is it possible in Rust to declare variables like:
if want_edge: let mut cycles: Vec<Vec<G::EdgeId>> = Vec::new(); else: let mut cycles: Vec<Vec<G::NodeId>> = Vec::new();
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 (G::EdgeId || G::NodeId) without having to wrap two similar processes under if and else statements, which could be a bit redundant.
I could, however, change the enum so that it represents the individual nodes/edges (instead of Vec<Vec<T>> for each type) and make the function return an instance of Vec<Vec<EdgeOrNodes<E, N>>> and then unwrap the values of said Vec individually.
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.
if there a reason to declare both a cycles_edges and a cycles_nodes if from what I understand we only return one or the other? Is it possible in Rust to declare variables like:
if want_edge: let mut cycles: Vec<Vec<G::EdgeId>> = Vec::new(); else: let mut cycles: Vec<Vec<G::NodeId>> = Vec::new();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 (
G::EdgeId || G::NodeId) without having to wrap two similar processes under if and else statements, which could be a bit redundant.I could, however, change the enum so that it represents the individual nodes/edges (instead of
Vec<Vec<T>>for each type) and make the function return an instance ofVec<Vec<EdgeOrNodes<E, N>>>and then unwrap the values of saidVecindividually.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.
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.
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.
- 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.NodeIdin the format(source, target).weight()method from EdgeRef returns a reference and cannot be copied over.