Add layers function to rustworkx-core

[raynelfss]

Attempts to resolve #1167

Summary

These commits bring the layers function down to rustworkx-core so that it can be used within the rust interface. This function was only available through a python interface previously.

Changes

• rustworkx.rustworkx-core.layers layers(): This function returns a list of subgraphs whose nodes are disjointed.
  • The main difference between this and its python counterpart is that this function only returns a list of NodeId instances. To get the weights/data stored in these nodes, the user can iterate through the graph accessing the weights based on the instance of NodeId.
• rustworkx.dag_algo layers(): This function is similar to the one in rustworkx-core, but allows the user to choose between getting node indices and node weights/data.
  • The logic has been updated to use the rustworkx-core version, but its behavior is identical to what it was previously.

Future directions:

• To return an Iterator instance from rustworkx-core, instead of a Vec<Vec<G::NodeId>> to prevent from unnecessary allocations. Added!
• Open to suggestions.

[coveralls]

Pull Request Test Coverage Report for Build 9685577238

Details

• 108 of 121 (89.26%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.06%) to 95.388%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dag_algo/mod.rs	34	36	94.44%
rustworkx-core/src/err.rs	0	3	0.0%
rustworkx-core/src/dag_algo.rs	74	82	90.24%

Totals
Change from base Build 9672352036:	-0.06%
Covered Lines:	17972
Relevant Lines:	18841

---

💛 - Coveralls

[mtreinish]

Thanks for doing this, just some quick high level comments from a first skim over the code.

The other things are can you add a couple of other rust space tests so we have some more dedicated coverage of the rust api (more than just the doctest) and also a release note?

[mtreinish]

Just some quick comments from a first skim through the PR.

[mtreinish]

Thanks for doing this, it's a good start. I think in this case we should make layers() a true iterator because it doesn't need to do a complete traversal to compute the final output. We can and should return this layer by layer. The reason we couldn't do that in python is we can't put references in a pyclass for the iteration, but in a Rust API we don't have that limitation. I left an inline code suggestion to start you down that path.

The other thing is I'm thinking we may not need to return a Result<> for this function, the only error condition we returned before was input checking we needed for Python interoperability, but doesn't really feel right in a rust API. You added a cycle check in this case which we could error for, but I'd also be fine just panicking in that case because this function doesn't work if there are cycles. Or alternatively just stop iterating when we reach a cycle. In either case we could just document that behavior and avoid the Result and I think either would probably be acceptable because this function is really only for DAGs so you shouldn't be passing a graph with cycles.

[raynelfss]

Thank you for the reviews @mtreinish, I believe this PR should be in a better position after the latest commtis. I managed to remove some of the overhead of iterating through the entire graph to find cycles, instead I keep track of all the layers being found, if one of them repeats, then a cycle is present. I thought of maybe doing it by index but, in some cases, the indices might repeat without it necessarily being a cycle.

let edge_list = vec![
  (0, 1),
 (1, 2),
 (2, 3),
 (3, 4),
];

let graph = DiGraph::<u32, u32>::from_edges(&edge_list);
let layers: Vec<Vec<NodeIndex>> = layers(&graph, vec![0.into(),1.into()]).collect();
let expected_layers: Vec<Vec<NodeIndex>> = vec![
    vec![0.into(),1.into()],
    vec![1.into(),2.into()],
    vec![2.into(),3.into()],
    vec![3.into(),4.into()],
    vec![4.into()]
];
assert_eq!(layers, expected_layers)

In this case, while the layers show repeated indices, they don't represent a cycle.

Also due to the nature of the graph being that when a node is removed the indices shift, therefore keeping the nodes within a range, if a node is provided outside that range (which can be found using graph.node_bound() -> usize), the graph doesn't contain that node. This is the only way I found that allowed me to check if a node is in the graph without having to collect all of the nodes.

As always there is room for improvement. If there's any other inconsitencies that anyone might find feel free to leave it as a review comment. Thanks!

[mtreinish]

This LGTM now thanks for the updates and thoroughness on this PR. I just have a few minor nits and questions inline. But otherwise I think this is ready to merge.

[mtreinish]

Do we need this explicitly, won't we just get a panic if we try to access node in neighbors_directed() below?

[raynelfss]

Not really. From the docs:

Produces an empty iterator if the node doesn’t exist.
Iterator element type is NodeIndex.

It only produces an empty iterator, therefore I had to introduce a way to make it panic if the index does not belong.

[mtreinish]

Is it possible to have a cycle here? Or are you protecting against duplicate entries in first_layer. If it's duplicates we should put that in the error message.

[raynelfss]

Just protecting against duplicates. I can make the warning more explicit though.

[mtreinish]

Same question here