adding line_graph and greedy_edge_color for undirected graphs

[alexanderivrii]

This is an attempt to start developing in Rust/Rustworkx by implementing the line_graph method that constructs a line graph from a given graph (the line graph contains a vertex for each edge of the original graph, with two vertices connected by an edge when the corresponding edges of the original graph meet at a vertex; see https://en.wikipedia.org/wiki/Line_graph), and by implementing the graph_greedy_edge_color function that takes a graph and edge-colors it by first constructing its line graph, and then greedy vertex-coloring this line graph.

This code still has many problems, but I would be happy to get some initial feedback:

• How do we decide that some code belong to rustworkx-core/src vs. src. For instance, the cartesian product code (which I was trying to adapt to construct line graphs in Rust) is not in the core. Should this edge coloring code be organized differently (for example, be a part of a separate file)?

• What should be the API for line_graph? Currently, it's a new graph and a hash map from the edges of the original graph to vertices of the new graph (somewhat similarly to cartesian_product), does this makes sense (actually, cartesian_product seems to use some kind of custom hash map)? Is it correct to use HashMap? Would it make sense to lift line_graph to a Python method as well?

• What would be the right way to get all pairs of edges from a given node (in Rust)? The current code is super-ugly. Search shows that one can use itertools::Itertools; for this, however when I do this I get a compiler error "use of undeclared crate or module itertools".

• Where I am completely stuck right now. Given a graph, I can construct its line graph and call graph_greedy_color on that graph. However, I still need to remap the returned coloring to a map from the edges of the original graph to colors. However, the coloring is a PyObject (graph_greedy_color creates a PyDict and returns Ok(out_dict.into()) and I don't understand how to extract a map out of that.

[mtreinish]

How do we decide that some code belong to rustworkx-core/src vs. src. For instance, the cartesian product code (which I was trying to adapt to construct line graphs in Rust) is not in the core. Should this edge coloring code be organized differently (for example, be a part of a separate file)?

In general we put things in rustworkx-core if we think it provides broader value to any rust user of the library. The consumers of the core crate is other rust code and it's how we share common functionality to other rust users (which includes rustworkx and qiskit-terra). In general we probably should have more of rustworkx's algorithms in rustworkx-core (assuming they can be written as generic rust functions instead of needing the Python specifics), but the core crate is much newer than the library and we haven't migrated everything over yet. We also don't have a requirement that new functions go through core as it's often trivial to port them if needed. In this specific case if you think there is value in using the cartessian product code more broadly we totally should move it to rustworkx-core.

What should be the API for line_graph? Currently, it's a new graph and a hash map from the edges of the original graph to vertices of the new graph (somewhat similarly to cartesian_product), does this makes sense (actually, cartesian_product seems to use some kind of custom hash map)? Is it correct to use HashMap? Would it make sense to lift line_graph to a Python method as well?

I think a graph object and a hash map to map the edges makes sense to me. It gives you all the necessary information to work with the output from the function. I do think it's probably good to add a python API for it as well, since it might be more broadly useful for the library. We can reuse the ProductNodeMap return type that we have for cartesian_product for the return from the python function. (that's just a custom return type that defers the type conversion/copy from rust -> python until we access the data in Python, it makes the return of the function faster but adds a little bit of overhead of getitem in python space)

What would be the right way to get all pairs of edges from a given node (in Rust)? The current code is super-ugly. Search shows that one can use itertools::Itertools; for this, however when I do this I get a compiler error "use of undeclared crate or module itertools".

When you say from a node in a directed graph does this mean outgoing edges only or do you want both incoming and outgoing edges? Either way I'd probably use an iterator like:

let edge_pairs: Vec<[NodeIndex; 2]> = graph.graph
    .edges(node_index)
    .map(|edge| [edge.source(), edge.target()|)
    .collect();

which for undirected graphs is all edges and for directed graphs is only outgoing edges from node_index. If you want both incoming and outgoing edges for a directed graph I would just call edges_directed twice and use Iterator::chain() to combine the iterators: https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.chain

Where I am completely stuck right now. Given a graph, I can construct its line graph and call graph_greedy_color on that graph. However, I still need to remap the returned coloring to a map from the edges of the original graph to colors. However, the coloring is a PyObject (graph_greedy_color creates a PyDict and returns Ok(out_dict.into()) and I don't understand how to extract a map out of that.

This points to an issue with the graph_greedy_color() functions API. When I wrote it I didn't think anyone would ever need to consume it from rust so I optimized it for Python usage and that's why it manually builds a Python object directly. We'll have to refactor the function a bit first (probably in a separate PR before this one) to call it efficiently from rust. The options I see for doing this are:

1. Move the core algorithm to rustworkx-core and then update the rustworkx caller to just call that, this has the advantage of letting any rust caller (this PR included) have access to the coloring algorithm.
2. Split the algorithm into an inner function and outer function. The inner function will return a hashmap and we can call that from this PR and then the existing outer function will return to python as it does now. This is basically the same work as rustworkx-core without moving the inner function to the core crate (so I would prefer we did 1 instead).
3. Adjust the return type of the function to be a rust type (like an IndexMap) and let PyO3 convert to a Python object for us. This will let rust callers like this PR get a rust native type but Python will still see a dictionary. The reason this wasn't done is IIRC when I was tuning that function I found it was faster to construct the dictionary manually like the code is doing now. But, re-usability is worth a small performance hit.

It is also technically possible to use PyO3's API to get the fields back from the Python object returned but it's not the most ergonomic API, it's also going to have a large amount of overhead because we have to call back to python for everything. It'll be a lot more efficient to do one of the above 3.

[jlapeyre]

What is the immediate application for Qiskit? I'd want to know this before commenting on the API and layers.

I think separating the pure-rust and Python layers should be the default. It could be violated for specific reasons.

[alexanderivrii]

Thanks @mtreinish, this is really-really helpful.

I agree with @jlapeyre's that it would be best to decide on the Python (Qiskit) API first. Adding to the python code in #854, I was thinking something along the following lines (with the changes in this PR, the code below can already be executed). Does this look like what we want?

import retworkx as rx
rxgraph = rx.PyGraph()
edge_list = [tuple(e) for e in edges]
rxgraph.extend_from_edge_list(edge_list)
edge_colors = rx.graph_greedy_edge_color(rxgraph)  # edge_colors is a dict from edge index to color
colors = {edge(rxgraph.get_edge_endpoints_by_index(i)[0], rxgraph.get_edge_endpoints_by_index(i)[1]):c for i, c in edge_colors.items()}
plot_colors(colors)

Or another possible way to visualize edge-colored graphs:

def edge_attr_fn(edge):
    attr_dict = {}
    if edge == 0:
        attr_dict["color"] = "orange"
    elif edge == 1:
        attr_dict["color"] = "blue"
    elif edge == 2:
        attr_dict["color"] = "green"
    else:
        attr_dict["color"] = "red"
    return attr_dict

for edge in rxgraph.edge_indices():
    rxgraph.update_edge_by_index(edge, edge_colors[edge])

from rustworkx.visualization import graphviz_draw
graphviz_draw(rxgraph, edge_attr_fn=edge_attr_fn, method="neato")

[alexanderivrii]

@mtreinish, I think that I was able to move the greedy_color function into rustworkx-core, could I please get your initial feedback on the code in rustworkx-core/src/coloring.rs?

In addition, a few specific questions:

• I keep changing my opinion on whether I should make a separate PR just on moving the greedy_color to core, which do you prefer?

• it is likely that not all of the G's traits used in greedy_color are required (as these were mostly copy-pasted from other files). Should I try to remove all such unnecessary traits?

[mtreinish]

@mtreinish, I think that I was able to move the greedy_color function into rustworkx-core, could I please get your initial feedback on the code in rustworkx-core/src/coloring.rs?

That looks good to me. The only things things really missing are the rust docs for the new function in rustworkx-core (the documentation format is different than the python side as we use rustdoc for the rust library, you can see the documentation about documentation here: https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html). Also include a usage example in the docs is good practice because we also get a doc test for free with doing that. Also, you can put the tests inline in the same coloring.rs module, we normally only make a separate test file if it's an integration test across multiple components

In addition, a few specific questions:

* I keep changing my opinion on whether I should make a separate PR just on moving the `greedy_color` to core, which do you prefer?

I think doing the migration to rustworkx-core in a separate PR makes the most sense. That should be easier to review, test, and merge in isolation as it should be mostly mechanical.

* it is likely that not all of the `G`'s traits used in `greedy_color` are required (as these were mostly copy-pasted from other files). Should I try to remove all such unnecessary traits?

Yeah, we don't want to overly constrain the trait requirements on the input graph. You can just try removing them one by one and the compiler will complain if it's actually being used.

[alexanderivrii]

Thanks, I have opened a PR #875 to migrate greedy_color to to rustworkx-core`.

[coveralls]

Pull Request Test Coverage Report for Build 5476551698

• 88 of 88 (100.0%) changed or added relevant lines in 5 files are covered.
• 4 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.03%) to 96.481%

---

Files with Coverage Reduction	New Missed Lines	%
rustworkx-core/src/connectivity/all_simple_paths.rs	1	97.44%
src/shortest_path/all_pairs_bellman_ford.rs	1	99.44%
src/shortest_path/all_pairs_dijkstra.rs	2	98.54%

Totals
Change from base Build 5470325765:	0.03%
Covered Lines:	15272
Relevant Lines:	15829

---

💛 - Coveralls

[alexanderivrii]

This should be ready for review. I have implemented the line_graph method inside the rustworkx_core (somehow it seemed good to have it there) and have added a python interface that allows to call rustworkx.graph_line_graph to construct the line graph of a given graph. I have also added the graph_greedy_edge_color method.

[mtreinish]

Overall this looks really good to me. Thanks for doing this! I just left two comments inline, the big one though is I think it'd be good to add greed_edge_color to rustworkx-core too just because everything else in this code path is already.

[mtreinish]

This LGTM, I left a couple of inline comments but nothing major worth blocking this over. Just two release note changes that we can easily do at the 0.14.0 release time and 1 alternative syntax using an iterator collect() instead of a for loop, but functionally I don't think it'll make any difference in that case so it's not worth changing.

[mtreinish]

More a note for myself when we release 0.14.0 it'll be good to have this visualized by calling mpl_draw() in the release notes instead of asserts. But I'll just do this as part of the release prep.

[mtreinish]

More a note for myself when we release 0.14.0 it'll be good to have this visualized by calling mpl_draw() in the release notes instead of asserts. But I'll just do this as part of the release prep.

[mtreinish]

Not that it matters from a performance perspective, but I think this could be rewritten as:

Suggested change

	let mut edge_colors: DictMap<G::EdgeId, usize> = DictMap::with_capacity(graph.edge_count());
	for edge in graph.edge_references() {
	let edge_index = edge.id();
	let node_index = edge_to_node_map.get(&edge_index).unwrap();
	let edge_color = colors.get(node_index).unwrap();
	edge_colors.insert(edge_index, *edge_color);
	}
	edge_colors
	graph
	.edge_references()
	.map(|edge| {
	let edge_index = edge.id();
	let node_index = edge_to_node_map.get(&edge_index).unwrap();
	let edge_color = colors.get(node_index).unwrap();
	(edge_index, *edge_color)
	})
	.collect()