Implementation of cycle_basis_edges

[raynelfss]

Aims to fix #551

• Inspired by the existing procedure cycle_basis.
• Modified to return a tuple of NodeId in the format (source, target).
• Added API calls so that method works in Python.
• TO-DO: return weights:
  • weight() method from EdgeRef returns a reference and cannot be copied over.

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

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.}

[raynelfss]

Results from testing in Python:

[coveralls]

Pull Request Test Coverage Report for Build 5946399193

• 87 of 102 (85.29%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.07%) to 96.367%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rustworkx-core/src/connectivity/cycle_basis.rs	79	94	84.04%

Totals
Change from base Build 5940303796:	-0.07%
Covered Lines:	15465
Relevant Lines:	16048

---

💛 - Coveralls

[mtreinish]

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).

[mtreinish]

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.

[raynelfss]

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?

[mtreinish]

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.

[raynelfss]

I see, I will make those changes accordingly.

[danielleodigie]

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

[raynelfss]

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.

[danielleodigie]

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.

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

[danielleodigie]

LGTM!

[mtreinish]

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.

[mtreinish]

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.

[raynelfss]

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.

[mtreinish]

This is looking good, thanks for the fast updates on this, I have a few more comments inline.

[mtreinish]

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.

[mtreinish]

You need to split this into 2 release notes, one for the rustworkx-core function and one for the python side function being added.

Suggested change

	- |
	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.