Add `is_semi_connected()` to `rustworkx` #1215

Procatv · 2024-06-10T16:41:58Z

Implemented the is_semi_connected function, I don't believe it's fully ready to go so I would appreciate feedback regarding my changes, and any more tests I can add, and etc. Followed implementation from definition: https://networkx.org/documentation/stable/reference/algorithms/generated/networkx.algorithms.components.is_semiconnected.html#networkx.algorithms.components.is_semiconnected

Fixes #446

coveralls · 2024-06-10T22:25:25Z

Pull Request Test Coverage Report for Build 9452263211

Details

28 of 29 (96.55%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.001%) to 95.871%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/connectivity/mod.rs	27	28	96.43%

Totals
Change from base Build 9450395122:	0.001%
Covered Lines:	17414
Relevant Lines:	18164

💛 - Coveralls

IvanIsCoding

Rust code is looking good BUT I think we can still take advantage of multi-threading to boost our speedup compared to sequential libraries. Let me know if you have more questions.

The tests are good but I think we should add some larger inputs. Our generators can help with that

tests/digraph/test_is_semi_connected.py

src/connectivity/mod.rs

IvanIsCoding

LGTM, just leaving one final comment about removing has_path_connecting for something simpler

src/connectivity/mod.rs

IvanIsCoding

I left a comment on a test case that is wrong, the condition is simpler, we check only one direction not both

src/connectivity/mod.rs

tests/digraph/test_is_semi_connected.py

src/connectivity/mod.rs

Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>

IvanIsCoding · 2024-06-18T04:01:59Z

I fixed clippy due to the Rust update to 1.79, sorry for the inconvenience. Let's hope CI passes now.

coveralls · 2024-06-18T04:12:35Z

Pull Request Test Coverage Report for Build 9558747991

Details

53 of 57 (92.98%) changed or added relevant lines in 12 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.05%) to 95.879%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rustworkx-core/src/planar/lr_planar.rs	0	1	0.0%
src/connectivity/mod.rs	35	36	97.22%
rustworkx-core/src/generators/random_graph.rs	1	3	33.33%

Totals
Change from base Build 9473947759:	0.05%
Covered Lines:	17426
Relevant Lines:	18175

💛 - Coveralls

IvanIsCoding · 2024-06-18T04:24:47Z

So some test cases were not correct, I pushed the test cases I believe are correct (you can double check with NetworkX, every path graph should be semi connected). So let's focus on finding the bug and making the test cases pass.

Procatv · 2024-06-18T16:01:24Z

So some test cases were not correct, I pushed the test cases I believe are correct (you can double check with NetworkX, every path graph should be semi connected). So let's focus on finding the bug and making the test cases pass.

I believe test_is_semi_connected_disconnected_graph should stay false given that there are no edges in the graph.
I made a mistake with test_is_semi_connected_false I was copying the test provided in the is_semiconnected() page and realized that in networkx adding nodes using path_graph starts from 0 to n-1, while add_nodes_from in rustworkx is from 1 to n, so the implementation was working correctly, but the test itself was wrong if I wanted to assert false (3 should be 4)
Also I believe that we should still check edges in both directions, one direction checking only works for all other graphs except any directed_path_graph which makes sense given that we only check one direction with has_edge -> when I implement the above (bidirectional edge check and test_is_semi_connected_disconnected_graph to false) changes all tests pass

IvanIsCoding · 2024-06-19T02:51:49Z

So some test cases were not correct, I pushed the test cases I believe are correct (you can double check with NetworkX, every path graph should be semi connected). So let's focus on finding the bug and making the test cases pass.

I believe test_is_semi_connected_disconnected_graph should stay false given that there are no edges in the graph.

I made a mistake with test_is_semi_connected_false I was copying the test provided in the is_semiconnected() page and realized that in networkx adding nodes using path_graph starts from 0 to n-1, while add_nodes_from in rustworkx is from 1 to n, so the implementation was working correctly, but the test itself was wrong if I wanted to assert false (3 should be 4)

Also I believe that we should still check edges in both directions, one direction checking only works for all other graphs except any directed_path_graph which makes sense given that we only check one direction with has_edge -> when I implement the above (bidirectional edge check and test_is_semi_connected_disconnected_graph to false) changes all tests pass

The disconnected graph was my mistake, but graphs like A -> B <- C are not semi-connected but with the wrong check the code mistakenly reported them as so.

We can use the floyd_warshall functions with the definition in the tests to estabilish a "ground truth" and work from there. For A -> B <- C there is no path from A to C so the algorithm should report it as not connected.

IvanIsCoding

After thinking about the problem more thoroughly, I am jumping in to implement is_semi_connected in a way that reuses almost 100% of our existing code.

I apologize for not thinking about it earlier, but the condition of u->v for each vertex of the NetworkX implementation is simply checking that there is a path covering all vertices in the DAG. It turns out that longest_path that we recently moved to be pure Rust code in #1192 does exactly that! So the NetworkX check is equivalent to seeing if there is a longest path with N-1 nodes in a DAG of N nodes.

Thanks a lot of for writing the original code! I know it ended up not being the original idea but this process led us to implement the least amount of code possible, which will be simpler to maintain in the future.

IvanIsCoding · 2024-06-19T03:58:52Z

src/connectivity/mod.rs

+    match longest_path(&condensed, weight_fn) {
+        Ok(path_len) => {
+            if let Some(path_len) = path_len {
+                Ok(path_len.1 == n - 1)
+            } else {
+                Ok(false)
+            }
+        }
+        Err(_) => Err(PyValueError::new_err("Graph could not be condensed")),
+    }


This is equivalent to checking the chain of u->v from NetworkX

coveralls · 2024-06-19T04:10:47Z

Pull Request Test Coverage Report for Build 9575905154

Details

25 of 27 (92.59%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.005%) to 95.86%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/connectivity/mod.rs	24	26	92.31%

Totals
Change from base Build 9575355125:	-0.005%
Covered Lines:	17413
Relevant Lines:	18165

💛 - Coveralls

Procatv added 7 commits June 10, 2024 10:15

added release note

d153495

added is_semi_connected implementation

95820b6

testing files for is_semi_connected

517fe52

formatting changes

508fa2e

formatting with lint

f7b78d9

added missing is_semi_connected module

6bf00b7

Merge branch 'main' into feature/is_semi_connected

b031b5b

Procatv changed the title ~~Implemented :func ~rustworkx.is_semi_connected()~~ Add ~rustworkx.is_semi_connected() Jun 10, 2024

Procatv changed the title ~~Add ~rustworkx.is_semi_connected()~~ Add is_semi_connected to rustworkx Jun 10, 2024

Procatv changed the title ~~Add is_semi_connected to rustworkx~~ Add is_semi_connected() to rustworkx Jun 10, 2024

IvanIsCoding reviewed Jun 13, 2024

View reviewed changes

tests/digraph/test_is_semi_connected.py Show resolved Hide resolved

src/connectivity/mod.rs Outdated Show resolved Hide resolved

Procatv added 3 commits June 13, 2024 18:27

added more large graph tests

9533501

updated function

b4a2f75

Merge branch 'main' into feature/is_semi_connected

b19bd9f

IvanIsCoding reviewed Jun 14, 2024

View reviewed changes

src/connectivity/mod.rs Outdated Show resolved Hide resolved

Procatv added 2 commits June 14, 2024 15:33

updated function

07b562c

fixed missing &&

7bc6dd5

IvanIsCoding reviewed Jun 15, 2024

View reviewed changes

src/connectivity/mod.rs Outdated Show resolved Hide resolved

tests/digraph/test_is_semi_connected.py Outdated Show resolved Hide resolved

IvanIsCoding marked this pull request as ready for review June 15, 2024 01:41

IvanIsCoding reviewed Jun 15, 2024

View reviewed changes

src/connectivity/mod.rs Outdated Show resolved Hide resolved

src/connectivity/mod.rs Outdated Show resolved Hide resolved

Procatv and others added 9 commits June 17, 2024 09:17

Update src/connectivity/mod.rs

89148c4

Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>

Update tests/digraph/test_is_semi_connected.py

3ee774e

Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>

Update src/connectivity/mod.rs

feec263

Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>

added error msg

56f091d

updated tests

5b814e4

updated formatting

09fda55

resolving conflicts

31e0af6

removal of duplicate

766ae3a

Issue with cargo clippy

83549e9

Fix clippy

ed31f09

Fix tests

9e62873

Merge branch 'main' into feature/is_semi_connected

b669ee9

IvanIsCoding and others added 4 commits June 18, 2024 23:40

Add ground truth semi-connected implementation with Floyd-Warshall

dc37bf9

Reuse code from dag_longest_path

1426ae8

Cargo fmt

f32a61e

Merge branch 'main' into feature/is_semi_connected

c8bbd09

IvanIsCoding approved these changes Jun 19, 2024

View reviewed changes

IvanIsCoding added the automerge Queue a approved PR for merging label Jun 19, 2024

IvanIsCoding merged commit bdec3ff into Qiskit:main Jun 19, 2024
28 checks passed

IvanIsCoding mentioned this pull request Jun 19, 2024

Miscellaneous test improvements #1219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `is_semi_connected()` to `rustworkx` #1215

Add `is_semi_connected()` to `rustworkx` #1215

Procatv commented Jun 10, 2024

coveralls commented Jun 10, 2024 •

edited

Loading

IvanIsCoding left a comment

IvanIsCoding left a comment

IvanIsCoding left a comment

IvanIsCoding commented Jun 18, 2024

coveralls commented Jun 18, 2024 •

edited

Loading

IvanIsCoding commented Jun 18, 2024

Procatv commented Jun 18, 2024 •

edited

Loading

IvanIsCoding commented Jun 19, 2024

IvanIsCoding left a comment •

edited

Loading

IvanIsCoding Jun 19, 2024

coveralls commented Jun 19, 2024 •

edited

Loading

Add is_semi_connected() to rustworkx #1215

Add is_semi_connected() to rustworkx #1215

Conversation

Procatv commented Jun 10, 2024

coveralls commented Jun 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9452263211

Details

💛 - Coveralls

IvanIsCoding left a comment

Choose a reason for hiding this comment

IvanIsCoding left a comment

Choose a reason for hiding this comment

IvanIsCoding left a comment

Choose a reason for hiding this comment

IvanIsCoding commented Jun 18, 2024

coveralls commented Jun 18, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9558747991

Details

💛 - Coveralls

IvanIsCoding commented Jun 18, 2024

Procatv commented Jun 18, 2024 • edited Loading

IvanIsCoding commented Jun 19, 2024

IvanIsCoding left a comment • edited Loading

Choose a reason for hiding this comment

IvanIsCoding Jun 19, 2024

Choose a reason for hiding this comment

coveralls commented Jun 19, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9575905154

Details

💛 - Coveralls

Add `is_semi_connected()` to `rustworkx` #1215

Add `is_semi_connected()` to `rustworkx` #1215

coveralls commented Jun 10, 2024 •

edited

Loading

coveralls commented Jun 18, 2024 •

edited

Loading

Procatv commented Jun 18, 2024 •

edited

Loading

IvanIsCoding left a comment •

edited

Loading

coveralls commented Jun 19, 2024 •

edited

Loading