-
Notifications
You must be signed in to change notification settings - Fork 150
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
Add connected components to retworkx-core #595
Conversation
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 a good start, thanks for doing this. I know this is still in progress, but I left a few inline comments all 3 functions need the same changes I think. Also it'd be good if we deleted the retworkx
crate's version of these functions and just updated the python interface to call the retworkx-core
version you're adding here.
Pull Request Test Coverage Report for Build 2360696828
💛 - Coveralls |
@mtreinish Remaining is,
|
…workx into core_connected_components
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.
With current trait restrictions we can't really remove retworkx's crate version of these functions since they also work for directed StableGraph
s.
/// | ||
/// :param Graph graph: The input graph to find the number of connected | ||
/// components for. | ||
/// | ||
/// :return: The number of connected components. | ||
/// :rtype: usize |
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.
/// | |
/// :param Graph graph: The input graph to find the number of connected | |
/// components for. | |
/// | |
/// :return: The number of connected components. | |
/// :rtype: usize |
Can you elaborate on this for me? What are the trait restrictions? Thanks. |
@enavarro51 |
and remove retworkx/src/connectivity/conn_components.rs and modify retworkx/src/connectivity/mod.rs to point to retworkx/retworkx-core/src/connectivity/conn_components.rs
This latest version should resolve all the open review issues here. The following changes have been made,
|
…workx into core_connected_components
stack.push_front(start); | ||
|
||
while let Some(node) = stack.pop_front() { | ||
for succ in graph.neighbors(node) { |
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 not as simple as dropping GraphProp<EdgeType = Undirected>
restriction. It will give wrong output for directed graphs since the algorithm that finds the weakly connected components of a directed graph must visit the neighbors of a node in both directions (incoming and outgoing).
For example the following fails (ideally our retworkx tests should have caught this):
import retworkx
graph = retworkx.PyDiGraph()
graph.add_nodes_from(range(4))
graph.add_edges_from_no_data([
(3, 2), (2, 1), (1, 0)
])
comps = retworkx.weakly_connected_components(graph)
# Graph is weakly connected, so it should have just one component
assert len(comps) == 1
There are some ways to fix this:
- We can keep the old
retworkx
version and restrictGraphProp<EdgeType = Undirected>
in order to accept only undirected graphs inretworkx-core
, or - We can restrict
G: IntoNeighborsDirected
and:
for succ in graph.neighbors(node) { | |
for succ in graph | |
.neighbors_directed(node, Outgoing) | |
.chain(graph.neighbors_directed(node, Incoming)) | |
{ |
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 LGTM now, thanks for sticking with it and making the updates. I just have a few small nits inline but once those are fixed I think we're good to go on this. @georgios-ts might want to take another look too before we merge though
/// | ||
/// * `graph` - The graph object to run the algorithm on | ||
/// * `node` - The node index to find the connected nodes for | ||
/// * `graph.visit_map()` - The visit map for the graph |
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.
/// * `graph.visit_map()` - The visit map for the graph | |
/// * `discovered` - The visit map for the graph |
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.
Done in d5e5def.
use petgraph::{Incoming, Outgoing}; | ||
|
||
/// Given an graph, a node in the graph, and a visit_map, | ||
/// return the set of nodes connected to the given node. |
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'd probably also mention here that this is in breadth first order and treating all edges as undirected.
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.
Done in d5e5def.
src/connectivity/mod.rs
Outdated
let return_value: Vec<HashSet<usize>> = connectivity::connected_components(&graph.graph) | ||
.into_iter() | ||
.map(|res_map| res_map.into_iter().map(|x| x.index()).collect()) | ||
.collect(); | ||
return_value |
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'll probably be optimized away by the compiler but we don't need the return_value
variable:
let return_value: Vec<HashSet<usize>> = connectivity::connected_components(&graph.graph) | |
.into_iter() | |
.map(|res_map| res_map.into_iter().map(|x| x.index()).collect()) | |
.collect(); | |
return_value | |
connectivity::connected_components(&graph.graph) | |
.into_iter() | |
.map(|res_map| res_map.into_iter().map(|x| x.index()).collect()) | |
.collect() |
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.
Done in d5e5def.
src/connectivity/mod.rs
Outdated
let return_value: HashSet<usize> = | ||
connectivity::bfs_undirected(&graph.graph, node, &mut graph.graph.visit_map()) | ||
.into_iter() | ||
.map(|x| x.index()) | ||
.collect(); | ||
Ok(return_value) |
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.
let return_value: HashSet<usize> = | |
connectivity::bfs_undirected(&graph.graph, node, &mut graph.graph.visit_map()) | |
.into_iter() | |
.map(|x| x.index()) | |
.collect(); | |
Ok(return_value) | |
Ok(connectivity::bfs_undirected(&graph.graph, node, &mut graph.graph.visit_map()) | |
.into_iter() | |
.map(|x| x.index()) | |
.collect()) |
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.
Done in d5e5def.
src/connectivity/mod.rs
Outdated
let return_value: Vec<HashSet<usize>> = connectivity::connected_components(&graph.graph) | ||
.into_iter() | ||
.map(|res_map| res_map.into_iter().map(|x| x.index()).collect()) | ||
.collect(); | ||
return_value |
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.
let return_value: Vec<HashSet<usize>> = connectivity::connected_components(&graph.graph) | |
.into_iter() | |
.map(|res_map| res_map.into_iter().map(|x| x.index()).collect()) | |
.collect(); | |
return_value | |
connectivity::connected_components(&graph.graph) | |
.into_iter() | |
.map(|res_map| res_map.into_iter().map(|x| x.index()).collect()) | |
.collect() |
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.
Done in d5e5def.
This PR adds the
connected_components
andnumber_connected_components
functions to retworkx-core. Needs a release note and maybe further tests and docs.