Skip to content

Conversation

@eumiro
Copy link
Contributor

@eumiro eumiro commented Mar 21, 2025

This should help new users to discover related methods and understand the method signature with simple examples. I believe there's enough space on the API docs website for more material.

I didn't find the following methods: number_strongly_connected_components, is_strongly_connected, number_semi_connected_components, and semi_connected_components. Otherwise I'd like to cross-reference them in the related docstrings.

Also I found it a bit confusing that connected_components and weakly_connected_components return a list of sets, whereas strongly_connected_components returns a list of lists, although the concept is similar.

OT: Did you actually consider using pytest for tests?

@eumiro eumiro changed the title Add docstring for some graph connectivity methods Extend docstrings for some graph connectivity methods Mar 21, 2025
@coveralls
Copy link

Pull Request Test Coverage Report for Build 14000177981

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.321%

Totals Coverage Status
Change from base Build 13983200019: 0.0%
Covered Lines: 18457
Relevant Lines: 19363

💛 - Coveralls

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for adding the examples on the docstrings. I think our documentation suffers from lacks of examples.

It is mostly that when contributing new code, we already need to add Rust code + Python bindings + tests + type annotations + a release note. So by the time authors get to the docstring, they are tired. I like this approach of refining docstrings after the fact.

For missing methods: again, similar fate from #1397. @eumiro if you could send a new PR editing this file docs/source/api/algorithm_functions/connectivity_and_cycles.rst, that would fix the issue. I can't believe we had methods undocumented for so long!

@IvanIsCoding
Copy link
Collaborator

Re: pytest. Matthew has his own test runner that is stestr. I myself like the built-in unittest module and at work I mostly use abseil-py which has a test module built on top of the unittest module.

I don't foresee us migrating to pytest. I am quite happy with the existing Python test setup. If you want to spend time contributing tests (which is welcome!), check #587 and #1390, they are mostly about Rust. #1316 is a Python-only test issue if you want to try, currently our matplotlib tests are close to non-existant.

@IvanIsCoding IvanIsCoding added this pull request to the merge queue Mar 22, 2025
@IvanIsCoding
Copy link
Collaborator

Last but not least: API inconsitencies. The methods were added by different people at different times. I don't think when Matthew wrote the earlier versions, he thought we'd have as big of a user base.

We have also a policy of avoiding breaking changes at all costs. For example, we have purposefully kept some API mistakes like having a sub-optimal order of Python arguments because changing the order would break users.

We might fix those things for 1.0 one day, so maybe comment on #1000 some improvements you can think of. Or just open a new issue.

Merged via the queue into Qiskit:main with commit 5f10121 Mar 22, 2025
31 checks passed
@eumiro eumiro deleted the docstring-graph-connectivity branch March 22, 2025 04:41
@eumiro
Copy link
Contributor Author

eumiro commented Mar 22, 2025

Thanks for the feedback, @IvanIsCoding . I find Rustworkx very useful for some of my use cases and while I approximately know, what I expect from my graphs, I do not always know the names of the methods/concepts, so I have to search around docs a lot (and learn stuff at the same time!). This is how I got the motivation to improve the docs for the *_connected_* methods and write some examples.

Am I searching in the wrong place? I cannot find the four missing methods from my top post at all. The only place where I find number_strongly_connected_components is in tests/digraph/test_strongly_connected.py and even that one uses the verbose strongly_connected_components to match against a list. Is it possible they do not exist at all?

I first thought these methods were not commonly used in the graph theory, but while networkx does not have the two *semi_connected* methods, it has both *strongly_connected* methods. Poor man's solution would be to use strongly_connected_components to count the number_strongly_connected_components and bool it to is_strongly_connected, but I believe there's some smarter shortcut.

And I understand your preference for unittest. I find pytest code more straightforward and easier to read (and therefore well suitable even for docs examples), but that's just personal taste and no reason to change working system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants