Implement condensation graph generation #1337

kazuki0824 · 2024-12-08T17:02:50Z

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 petgraph implementation 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.

I ran rustfmt locally
I have added the tests to cover my changes.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

coveralls · 2024-12-09T16:19:46Z

Pull Request Test Coverage Report for Build 12451773226

Details

58 of 64 (90.63%) changed or added relevant lines in 2 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.03%) to 95.807%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/connectivity/mod.rs	57	63	90.48%

Files with Coverage Reduction	New Missed Lines	%
rustworkx-core/src/generators/random_graph.rs	2	85.04%

Totals
Change from base Build 12365971131:	-0.03%
Covered Lines:	18417
Relevant Lines:	19223

💛 - Coveralls

kazuki0824 · 2024-12-22T05:58:24Z

@IvanIsCoding can anyone review this code?

IvanIsCoding · 2024-12-22T06:06:46Z

@IvanIsCoding can anyone review this code?

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.

kazuki0824 · 2024-12-22T13:14:42Z

@IvanIsCoding can anyone review this code?

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.

alexanderivrii · 2024-12-29T13:52:46Z

src/connectivity/mod.rs

+    g: Graph<N, E, Ty, Ix>,
+    make_acyclic: bool,
+    sccs: Option<Vec<Vec<usize>>>,
+) -> StableGraph<PyObject, PyObject, Ty, Ix>


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?

alexanderivrii · 2024-12-29T13:53:38Z

tests/digraph/test_strongly_connected.py

+        self.graph.add_edge(self.node_h, self.node_e, "h->e")  # サイクル: e -> f -> g -> h -> e
+
+    def test_condensation(self):
+        # condensation関数を呼び出し


It would be nice to translate all comments to English.

alexanderivrii

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).

IvanIsCoding

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.

IvanIsCoding · 2024-12-30T18:16:19Z

src/connectivity/mod.rs

+        check_cycle: false,
+        node_removed: false,
+        multigraph: true,
+        attrs: py.None(),


Either the attrs should contain the node mapping like in NetworkX. Or we need to update the signature to return a mapping

IvanIsCoding · 2024-12-30T18:16:40Z

src/connectivity/mod.rs

+    py: Python,
+    graph: &digraph::PyDiGraph,
+    sccs: Option<Vec<Vec<usize>>>,
+) -> digraph::PyDiGraph {


Sibling comment to either update the signature or return the node mapping somewhere else

IvanIsCoding · 2024-12-30T18:18:56Z

src/connectivity/mod.rs

+    g: Graph<N, E, Ty, Ix>,
+    make_acyclic: bool,
+    sccs: Option<Vec<Vec<usize>>>,
+) -> StableGraph<PyObject, PyObject, Ty, Ix>


Suggested change

) -> StableGraph<PyObject, PyObject, Ty, Ix>

) -> StablePyGraph<Directed>

Stick to the shared type alias for consistency with the other methods.

IvanIsCoding · 2024-12-30T18:20:01Z

src/connectivity/mod.rs

@@ -114,6 +114,79 @@ pub fn strongly_connected_components(graph: &digraph::PyDiGraph) -> Vec<Vec<usiz
        .collect()
 }

+fn condensation_inner<N, E, Ty, Ix>(


This function is called only once in the code, so I don't think you need to make it generic. It is fine to accept only one kind of type for each argument.

n.b: the feedback would be different if this wasn't a private function

IvanIsCoding · 2024-12-30T18:21:09Z

rustworkx/rustworkx.pyi

@@ -192,6 +192,7 @@ def number_connected_components(graph: PyGraph, /) -> int: ...
 def number_weakly_connected_components(graph: PyDiGraph, /) -> bool: ...
 def node_connected_component(graph: PyGraph, node: int, /) -> set[int]: ...
 def strongly_connected_components(graph: PyDiGraph, /) -> list[list[int]]: ...
+def condensation(graph: PyDiGraph, /, sccs=None) -> PyDiGraph: ...


Suggested change

def condensation(graph: PyDiGraph, /, sccs=None) -> PyDiGraph: ...

def condensation(graph: PyDiGraph, /, sccs: list[int] | None=...) -> PyDiGraph: ...

You don't specify the value here, just that the argument is optional because there is already a default

IvanIsCoding · 2024-12-30T18:23:34Z

src/connectivity/mod.rs

+            if source != target {
+                condensed.update_edge(source, target, edge.weight);
+            }
+        } else {


This is always false, remove this statmenet

IvanIsCoding · 2024-12-30T18:25:01Z

tests/digraph/test_strongly_connected.py

+
+class TestCondensation(unittest.TestCase):
+    def setUp(self):
+        # グラフをセットアップ


I appreciate the code comments but before submitting we'll either need to translate them to English

IvanIsCoding · 2024-12-30T18:26:47Z

tests/digraph/test_strongly_connected.py

+        self.graph.add_edge(self.node_a, self.node_b, "a->b")
+        self.graph.add_edge(self.node_b, self.node_c, "b->c")
+        self.graph.add_edge(self.node_c, self.node_d, "c->d")
+        self.graph.add_edge(self.node_d, self.node_a, "d->a")  # サイクル: a -> b -> c -> d -> a


Suggested change

self.graph.add_edge(self.node_d, self.node_a, "d->a") # サイクル: a -> b -> c -> d -> a

self.graph.add_edge(self.node_d, self.node_a, "d->a") # cycle : a -> b -> c -> d -> a

IvanIsCoding · 2024-12-30T18:29:15Z

tests/digraph/test_strongly_connected.py

+        self.graph.add_edge(self.node_e, self.node_f, "e->f")
+        self.graph.add_edge(self.node_f, self.node_g, "f->g")
+        self.graph.add_edge(self.node_g, self.node_h, "g->h")
+        self.graph.add_edge(self.node_h, self.node_e, "h->e")  # サイクル: e -> f -> g -> h -> e


Suggested change

self.graph.add_edge(self.node_h, self.node_e, "h->e") # サイクル: e -> f -> g -> h -> e

self.graph.add_edge(self.node_h, self.node_e, "h->e") # cycle: e -> f -> g -> h -> e

IvanIsCoding · 2024-12-30T18:32:04Z

tests/digraph/test_strongly_connected.py

@@ -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):


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

IvanIsCoding · 2024-12-30T18:35:54Z

Some notes to myself:

Add a release note for this once the code is ready (https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#release-notes)
Add a docstring to the method once the code is ready

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 from eed8af1 to fada9a1 Compare December 8, 2024 17:38

kazuki0824 marked this pull request as ready for review December 15, 2024 08:01

kazuki0824 force-pushed the feat/generate_condensation_graph branch from 8754ccd to 9c026b2 Compare December 15, 2024 08:06

Implement condensation tentatively

5045623

kazuki0824 force-pushed the feat/generate_condensation_graph branch from 57dc3fe to 5045623 Compare December 22, 2024 06:03

alexanderivrii reviewed Dec 29, 2024

View reviewed changes

IvanIsCoding reviewed Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement condensation graph generation #1337

Implement condensation graph generation #1337

kazuki0824 commented Dec 8, 2024 •

edited

Loading

coveralls commented Dec 9, 2024 •

edited

Loading

kazuki0824 commented Dec 22, 2024

IvanIsCoding commented Dec 22, 2024

kazuki0824 commented Dec 22, 2024

alexanderivrii Dec 29, 2024

alexanderivrii Dec 29, 2024

alexanderivrii left a comment

IvanIsCoding left a comment

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding Dec 30, 2024

IvanIsCoding commented Dec 30, 2024

	) -> StableGraph<PyObject, PyObject, Ty, Ix>
	) -> StablePyGraph<Directed>

	def condensation(graph: PyDiGraph, /, sccs=None) -> PyDiGraph: ...
	def condensation(graph: PyDiGraph, /, sccs: list[int] \| None=...) -> PyDiGraph: ...

	self.graph.add_edge(self.node_d, self.node_a, "d->a") # サイクル: a -> b -> c -> d -> a
	self.graph.add_edge(self.node_d, self.node_a, "d->a") # cycle : a -> b -> c -> d -> a

	self.graph.add_edge(self.node_h, self.node_e, "h->e") # サイクル: e -> f -> g -> h -> e
	self.graph.add_edge(self.node_h, self.node_e, "h->e") # cycle: e -> f -> g -> h -> e

Implement condensation graph generation #1337

Are you sure you want to change the base?

Implement condensation graph generation #1337

Conversation

kazuki0824 commented Dec 8, 2024 • edited Loading

coveralls commented Dec 9, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12451773226

Details

💛 - Coveralls

kazuki0824 commented Dec 22, 2024

IvanIsCoding commented Dec 22, 2024

kazuki0824 commented Dec 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderivrii left a comment

Choose a reason for hiding this comment

IvanIsCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanIsCoding commented Dec 30, 2024

kazuki0824 commented Dec 8, 2024 •

edited

Loading

coveralls commented Dec 9, 2024 •

edited

Loading