Enhance traversal docs #1375

barakatzir · 2025-01-29T21:55:19Z

This PR fixes #1361.
It add some elaboration to the traversal BFS and DFS functions, mainly with a more concrete pseudo-code.
I also fixed small doc errors in betweenness function's docstrings.
In the issue I also suggested doing a face-lift to the dijkstra_search. I can add this in to this PR in the style of the other docstrings changes I'm proposing If you'd like.

[v] I ran the nox -e docs and cargo doc to see that the result is good.
[v] I have read the CONTRIBUTING document.

CLAassistant · 2025-01-29T21:55:26Z

All committers have signed the CLA.

CLAassistant · 2025-01-29T21:55:26Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

IvanIsCoding · 2025-01-30T15:02:15Z

Thanks for submitting this! I will try to review it when I can, but again this is a welcome contribution.

coveralls · 2025-01-30T15:15:15Z

Pull Request Test Coverage Report for Build 13229065253

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 95.819%

Totals
Change from base Build 13105099943:	0.0%
Covered Lines:	18381
Relevant Lines:	19183

💛 - Coveralls

IvanIsCoding

This looks good to me with the caveat that I think for Python docstrings we should focus on the methods that highlight BFS/DFS only. I'd drop the iterator details

IvanIsCoding · 2025-02-02T21:16:10Z

rustworkx/__init__.py

-          end while
-
-    If an exception is raised inside the callback function, the graph traversal
+        def BFSIterator(G, I, F):


For Python I think it makes more sense to talk about BFS than BFSIterator. I'd rewrite it to keep the old signature BFS(G, s)

IvanIsCoding · 2025-02-02T21:17:34Z

rustworkx/__init__.py

 @_rustworkx_dispatch
 def dfs_search(graph, source, visitor):
-    """Depth-first traversal of a directed/undirected graph.
+    """Iterative depth-first traversal of a directed/undirected graph.


We could have a recursive DFS with something like https://crates.io/crates/recursive these days, so I am not opposed to saying this.

With that in mind, DFS order is implementation dependent and even the order we iterate edges is not promised to be stable so it is what it is.

I copied the Iterative prefix from the rustworkx-core since it made sense to me to differentiate this method from the DFS algorithm which has a single source vertex.
I didn't know about DFS (the CLRS ref you sent me was helpful!) but my friends who do know (both the algo and of CLRS) were confused a bit by how several source vertices are incorporated.
If the implementation detail of how several source vertices are incorporated into DFS is not guaranteed, the docs should probably mention that as well.

I will double check Georgios’ implementation in the source code and check.

I myself forgot the specifics, either the user passes it as an input or we pick an arbitrary node I think

I thought that the source vertices are added to the stack in reverse order. I know that it is not implemented like that, but it is close to it.
That way when you pop from the stack you start with the first source node as the user specified, etc...
This behavior is similar in the BFS (just with a stack instead of a queue).
This is one the behaviors that I wanted to clarify in the docs.
How about I rewrite the pseudocode like that? That way its clear how the many source nodes relate to the single source node in the DFS algo.

PS if the user does not specify the source the docs already specify the behavior:

If this is not specified then a source will be chosen arbitrarily and repeated until all components of the graph are searched.

I think if you rewrite the pseudo code to be similar to the implementation, users will like it. Go for it

I'll try to get to it tomorrow.

IvanIsCoding · 2025-02-02T21:18:53Z

rustworkx/__init__.py

-          color[u] := BLACK                     finish vertex u
-
-    If an exception is raised inside the callback function, the graph traversal
+        def DFSIterator(G, I, F):


Same comment from BFS

IvanIsCoding · 2025-02-02T21:20:01Z

rustworkx/__init__.py

+          color[s] = GRAY
+          time += 1
+          F.Discover(s, time)
+          Q = [s]                             # LIFO vertex queue


You can call it a stack

barakatzir · 2025-02-09T16:39:12Z

I reverted to the pseudo-code styling of the algorithms. I couldn't simplify the algorithms while staying similar to the implementation, so I thought going with pseudo-code conveys better that this does not follow the implementation.

I also redid the Dijkstra search as it was similarly unclear (to me).
Also, added an example - a python version of the one in rustworkx-core.

I fixed a minor type annotation issue in the universal dijkstra_search.

I checked that that cargo doc and nox -edocs give good results.

IvanIsCoding

Thanks for the documentation improvements!

I will merge #1380 with this and then lets check the docs. I revised but there are multiple repetitions of the same docstring (unfortunately, that is how the docs work). So let's see the end result in the website and judge. I have a feeling it will be better

…e-traversal-docs

* Enhance universal bfs_search docs and misc docs fixes * Enhance docs of non universal bfs_search * non-universal centrality doc fixes * New BFS pseudo-code in rustworkx-core * fixes after looking at docs * enhance dfs search docs * formatting * Fix docs publishing job * simplify pseudo-code * enhance dijksra_search docs * add dijkstra_search doc example * fix universal dijkstra_search type annotations * correct description: python code -> psuedo-code --------- Co-authored-by: Ivan Carvalho <[email protected]>

barakatzir added 6 commits January 29, 2025 20:40

Enhance universal bfs_search docs and misc docs fixes

6b809a3

Enhance docs of non universal bfs_search

5082e6a

non-universal centrality doc fixes

d56c96f

New BFS pseudo-code in rustworkx-core

1cac7ff

fixes after looking at docs

fa97374

enhance dfs search docs

fb33fda

IvanIsCoding added the documentation label Jan 29, 2025

formatting

5368728

IvanIsCoding approved these changes Feb 2, 2025

View reviewed changes

IvanIsCoding and others added 5 commits February 2, 2025 22:08

Fix docs publishing job

72ee5a6

simplify pseudo-code

69d4e02

enhance dijksra_search docs

19161f0

add dijkstra_search doc example

f878da6

fix universal dijkstra_search type annotations

ebb7049

Merge branch 'main' into enhance-traversal-docs

8b31b7b

barakatzir requested a review from IvanIsCoding February 9, 2025 17:07

correct description: python code -> psuedo-code

5a2be27

IvanIsCoding approved these changes Feb 9, 2025

View reviewed changes

Merge remote-tracking branch 'origin/fix-docs-publishing' into enhanc…

8e8722c

…e-traversal-docs

IvanIsCoding enabled auto-merge February 9, 2025 19:51

IvanIsCoding mentioned this pull request Feb 9, 2025

Bump GH Actions version to fix documentation publishing #1380

Closed

IvanIsCoding added this pull request to the merge queue Feb 9, 2025

Merged via the queue into Qiskit:main with commit df38b0a Feb 9, 2025
31 checks passed

barakatzir mentioned this pull request Feb 10, 2025

Panic on PruneSearch #1383

Open

Enhance traversal docs #1375

Enhance traversal docs #1375

Uh oh!

Conversation

barakatzir commented Jan 29, 2025

Uh oh!

CLAassistant commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Jan 29, 2025

Uh oh!

IvanIsCoding commented Jan 30, 2025

Uh oh!

coveralls commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 13229065253

Details

💛 - Coveralls

Uh oh!

IvanIsCoding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barakatzir commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IvanIsCoding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Jan 29, 2025 •

edited

Loading

coveralls commented Jan 30, 2025 •

edited

Loading

barakatzir commented Feb 9, 2025 •

edited

Loading