-
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
adding line_graph
and greedy_edge_color
for undirected graphs
#870
Conversation
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.
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
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
This points to an issue with the
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. |
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. |
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?
Or another possible way to visualize edge-colored graphs:
|
@mtreinish, I think that I was able to move the In addition, a few specific questions:
|
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
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.
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. |
Thanks, I have opened a PR #875 to migrate |
Pull Request Test Coverage Report for Build 5476551698
💛 - Coveralls |
line_graph
and greedy_edge_color
for undirected graphs
This should be ready for review. I have implemented the |
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.
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.
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 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.
assert out_graph.node_indices() == [0, 1, 2, 3] | ||
assert out_graph.edge_list() == [(3, 1), (3, 0), (1, 0), (2, 0), (2, 1)] | ||
assert out_edge_map == {edge_ab: 0, edge_ac: 1, edge_bc: 2, edge_ad: 3} |
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.
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.
|
||
graph = rx.generators.cycle_graph(7) | ||
edge_colors = rx.graph_greedy_edge_color(graph) | ||
assert edge_colors == {0: 0, 1: 1, 2: 0, 3: 1, 4: 0, 5: 1, 6: 2} |
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.
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.
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 |
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.
Not that it matters from a performance perspective, but I think this could be rewritten as:
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() |
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 thegraph_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 tocartesian_product
), does this makes sense (actually,cartesian_product
seems to use some kind of custom hash map)? Is it correct to useHashMap
? Would it make sense to liftline_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 moduleitertools
".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 aPyObject
(graph_greedy_color
creates aPyDict
and returnsOk(out_dict.into()
) and I don't understand how to extract a map out of that.