Implement condensation graph generation #1337
Conversation
kazuki0824
force-pushed
the
feat/generate_condensation_graph
branch
from
eed8af1 to
fada9a1
Compare
Pull Request Test Coverage Report for Build 14561252481Details
💛 - Coveralls |
kazuki0824
force-pushed
the
feat/generate_condensation_graph
branch
from
8754ccd to
9c026b2
Compare
|
@IvanIsCoding can anyone review this code? |
kazuki0824
force-pushed
the
feat/generate_condensation_graph
branch
from
57dc3fe to
5045623
Compare
Sorry I haven’t acknowledged the PR. I haven’t had time to review it. I will do the review after Christmas, it might still make the cut for rustworkx 0.16. |
Thank you for your quick response. |
src/connectivity/mod.rs
Outdated
| g: Graph<N, E, Ty, Ix>, | ||
| make_acyclic: bool, | ||
| sccs: Option<Vec<Vec<usize>>>, | ||
| ) -> StableGraph<PyObject, PyObject, Ty, Ix> |
There was a problem hiding this comment.
I am wondering if it would make sense to also return a map between the nodes of the condensed graph and the (sets of) nodes of the original graph, so that information computed for the condensed graph could be lifted to the original graph as well?
| self.graph.add_edge(self.node_h, self.node_e, "h->e") # サイクル: e -> f -> g -> h -> e | ||
|
|
||
| def test_condensation(self): | ||
| # condensation関数を呼び出し |
There was a problem hiding this comment.
It would be nice to translate all comments to English.
There was a problem hiding this comment.
This is not a review, but I was wondering: would it make sense to move this functionality to rustworkx-core? would it make sense to implement this both for directed and undirected graphs? would it make sense to return additional information relating the nodes of the original and the condensed graphs (see in-place comment).
There was a problem hiding this comment.
Overall the condensation logic looks good to me. I left comments that are mostly to simplify the existing code and make it more maintainable in the long run. I don't think you'll need to change much for the algorithm
I think the only debate left is how to return the node mappings. I personally like the short return type signature, so for me a graph attribute with the mapping seems like a good compromise.
src/connectivity/mod.rs
Outdated
| check_cycle: false, | ||
| node_removed: false, | ||
| multigraph: true, | ||
| attrs: py.None(), |
There was a problem hiding this comment.
Either the attrs should contain the node mapping like in NetworkX. Or we need to update the signature to return a mapping
src/connectivity/mod.rs
Outdated
| py: Python, | ||
| graph: &digraph::PyDiGraph, | ||
| sccs: Option<Vec<Vec<usize>>>, | ||
| ) -> digraph::PyDiGraph { |
There was a problem hiding this comment.
Sibling comment to either update the signature or return the node mapping somewhere else
| if source != target { | ||
| condensed.update_edge(source, target, edge.weight); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This is always false, remove this statmenet
| @@ -65,3 +65,55 @@ def test_number_strongly_connected_big(self): | |||
| node = G.add_node(i) | |||
| G.add_child(node, str(i), {}) | |||
| self.assertEqual(len(rustworkx.strongly_connected_components(G)), 200000) | |||
|
|
|||
|
|
|||
| class TestCondensation(unittest.TestCase): | |||
There was a problem hiding this comment.
The existing test case is excellent! But we need to add one test covering the case where we pass a list to sccs as an argument
|
Some notes to myself:
Feel free to do those on your own too. But I don't mind doing the documentation for you, it is fine to focus on the code. Again, thanks for your repeated contributions to rustworkx |
kazuki0824
force-pushed
the
feat/generate_condensation_graph
branch
2 times, most recently
from
affd659 to
162ef11
Compare
thanks @IvanIsCoding Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
thanks @IvanIsCoding Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
Update mod.rs
Update mod.rs Update mod.rs Update mod.rs
kazuki0824
force-pushed
the
feat/generate_condensation_graph
branch
from
162ef11 to
dc8b154
Compare
Update mod.rs ok ok wip Update mod.rs Reformat Reformat
|
@IvanIsCoding Now ready to review. I also added Rust doc and release notes. Please check them too. |
This pull request adds functionality to generate condensation graphs. The Rust implementation is copied and slightly modified from https://github.com/petgraph/petgraph because the original
petgraphimplementation cannot be applied as it is. Condensation graphs represent strongly connected components of a directed graph as single nodes. The update includes a new implementation and its test to support this feature.