Docs and consistency cleanup for rustworkx-core generators #784

enavarro51 · 2023-01-15T15:42:27Z

This PR cleans up the generators documentation and makes the code for the generators more consistent. It's based on #780 and should be merged after that one.

coveralls · 2023-01-15T16:10:44Z

Pull Request Test Coverage Report for Build 4092263202

151 of 153 (98.69%) changed or added relevant lines in 4 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.004%) to 97.093%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/generators.rs	141	143	98.6%

Files with Coverage Reduction	New Missed Lines	%
src/shortest_path/all_pairs_bellman_ford.rs	1	98.88%
src/shortest_path/all_pairs_dijkstra.rs	2	98.54%

Totals
Change from base Build 4079717942:	0.004%
Covered Lines:	13859
Relevant Lines:	14274

💛 - Coveralls

mtreinish · 2023-01-18T14:02:29Z

rustworkx-core/src/generators/binomial_tree_graph.rs

+            graph.update_edge(
+                graph.from_index(source_index),
+                graph.from_index(target_index),
+                default_edge_weight(),
+            );


This isn't quite the same because if the edge already exists we're replacing the weight with the output of default_edge_weight(). This won't matter if default_edge_weight() returns the same value every time, but there is no guarantee that will be the case.

Ah, missed that. Was thinking of the defaults as fixed. I can return to doing the find and then the add if not there.

Done in 63d2307.

mtreinish · 2023-01-18T14:04:19Z

rustworkx-core/src/generators/lollipop_graph.rs

 ///
-/// Arguments:
+/// .. note::


I don't think this syntax will work in rustdoc since it's markdown based. I'm not sure what the correct syntax is off the top of my head, we'll have to check the docs.

This passes the tests. Is there somewhere this would fail and not be tested?

It will just render in the docs as plain text ... note:: without anything special (as this is a sphinx directive and the rustworkx-core docs are built using rustdoc). You can test the output with cargo doc --open and see what the rendered docs look like: https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#rustworkx-core-documentation

Thanks for that info. I went back through all the docs in core and I think everything should be clean now. I removed all the markdown and made things prettier. Turns out the trick for displaying Ascii is to use text instead of rust for code-like sections.
Should be done in 63d2307 and b0e4dfc.

mtreinish · 2023-01-18T14:05:35Z

rustworkx-core/src/generators/mod.rs

@@ -26,7 +26,7 @@ mod path_graph;
 mod petersen_graph;
 mod star_graph;

-mod utils;
+pub mod utils;


Doesn't this expose rustworkx_core::generators::mod::utils as a public facing interface. Do we want to commit to the interface on the util functions there?

This is just there since rustworkx/src/steiner_tree.rs was using the pairwise fn in generators.rs. I moved pairwise to utils since it was being used by a couple of the generators, but no longer used in generators.rs. If you don't want pub here, the simplest would be to just duplicate pairwise in steiner_tree.

Ah, I missed that when I went through this earlier. I think that's fine but I just wonder about the rest of the utils module. If we only need pairwise maybe we should just export only that one function. I'm just worried about committing the API surface for what were previously internal functions in the python side.

So where should we put pairwise so it's accessible from both rustworkx and rustworkx-core?

Yeah, I think we should be conservative on what we make public here. We can always add more public things later if the need arises and we're confident with the API, but it's much harder to walk something back after we make it public.

I would keep the module private and change this to be pub use utils::pairwise or maybe make a top level public utils module that only contains the pairwise function for now.

I added a top-level utils with pairwise in it. In 5c3ea62.

mtreinish

Overall this LGTM, thanks for fixing all this. I just left a couple of inline comments on the python side docs. The only other thing is a small change to making rustworkx_core::generators::utils public so we limit it to just the pairwise function.

src/generators.rs

enavarro51 · 2023-02-01T18:39:06Z

Had a fairly major merge conflict with the pyo3 changes, which I think is all fixed, but you might want to take another look at generators.rs.

mtreinish

Overall I think this is mostly ready to go, just a couple small inline comments and questions. It's a bit tricky to review because the github webui shows some functions shifting positions, but I think it's all good

mtreinish · 2023-02-04T13:48:37Z

src/generators.rs

-///     :class:`~rustworkx.PyGraph` object will not be not be a multigraph and
-///     won't  allow parallel edges to be added. Instead
+/// :param bool multigraph: When set to ``False`` the output
+///     :class:`~rustworkx.PyDiGraph` object will not be not be a multigraph and


The github web ui diff is a bit weird so I might be reading this wrong, but shouldn't this be PyGraph because this is the undirected star graph variant?

src/generators.rs

mtreinish · 2023-02-04T13:52:18Z

src/generators.rs

 /// An ASCII diagram of the graph is given by:
 ///
-/// .. code-block:: console
+/// .. code-block:: text


Just curious the heavy hex ascii art uses text and heavy square uses console, was there a difference between the two?

Don't actually know why there is a difference. I did run into a problem on the rust docs side with the \'s on the heavy square drawing, but not sure that's related.

mtreinish

LGTM, thanks for making the updates

enavarro51 added 9 commits January 10, 2023 13:05

Initial r-ary

7b52d4b

Complete tests

3ed7207

Lint and name change

85d214e

Merge branch 'main' into gen_rary_graph

1775dff

Clippy

8230812

Merge branch 'main' into gen_rary_graph

0d88686

Code and docs consistency

4fe1f17

Docs changes to generators.rs

1764ba1

Reorder graphs in generators.rs

815ba3f

enavarro51 mentioned this pull request Jan 15, 2023

Add direct versions of 4 generators #785

Closed

enavarro51 added 3 commits January 15, 2023 13:53

Switch binomial tree to update_edge

52a2e9c

Merge main

7fc6e28

Update bidirectional docs

12fb467

mtreinish reviewed Jan 18, 2023

View reviewed changes

enavarro51 added 4 commits January 18, 2023 15:43

Cleanup rest of docs and return find_edge in binomial tree

63d2307

Second round on heavy docs

b0e4dfc

Merge branch 'main' into gen_cleanup

c4d0551

Minor doc change

4b8f1ae

mtreinish reviewed Feb 1, 2023

View reviewed changes

src/generators.rs Outdated Show resolved Hide resolved

src/generators.rs Outdated Show resolved Hide resolved

src/generators.rs Show resolved Hide resolved

enavarro51 added 3 commits February 1, 2023 08:57

Make core utils and cleanup docs

5c3ea62

Fmt

bef491b

Fix merge conflicts

663a334

enavarro51 mentioned this pull request Feb 1, 2023

Move core_number to rustworkx-core #795

Merged

mtreinish reviewed Feb 4, 2023

View reviewed changes

enavarro51 added 2 commits February 4, 2023 07:12

Merge branch 'main' into gen_cleanup

b2bcf0c

Text and PyGraph fixes

cc8be50

mtreinish approved these changes Feb 4, 2023

View reviewed changes

mtreinish added the automerge Queue a approved PR for merging label Feb 4, 2023

mergify bot merged commit b659159 into Qiskit:main Feb 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs and consistency cleanup for rustworkx-core generators #784

Docs and consistency cleanup for rustworkx-core generators #784

enavarro51 commented Jan 15, 2023

coveralls commented Jan 15, 2023 •

edited

mtreinish Jan 18, 2023

enavarro51 Jan 18, 2023

enavarro51 Jan 18, 2023

mtreinish Jan 18, 2023

enavarro51 Jan 18, 2023

mtreinish Jan 18, 2023

enavarro51 Jan 19, 2023

mtreinish Jan 18, 2023

enavarro51 Jan 18, 2023

mtreinish Jan 18, 2023

enavarro51 Jan 19, 2023

mtreinish Feb 1, 2023

enavarro51 Feb 1, 2023

mtreinish left a comment

enavarro51 commented Feb 1, 2023

mtreinish left a comment

mtreinish Feb 4, 2023

mtreinish Feb 4, 2023

enavarro51 Feb 4, 2023

mtreinish left a comment

Docs and consistency cleanup for rustworkx-core generators #784

Docs and consistency cleanup for rustworkx-core generators #784

Conversation

enavarro51 commented Jan 15, 2023

coveralls commented Jan 15, 2023 • edited

Pull Request Test Coverage Report for Build 4092263202

💛 - Coveralls

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

enavarro51 commented Feb 1, 2023

mtreinish 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

mtreinish left a comment

Choose a reason for hiding this comment

coveralls commented Jan 15, 2023 •

edited