Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document functions on module pages #10471

Merged
merged 7 commits into from
Jul 27, 2023

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented Jul 21, 2023

See #10455 for the motivation:

  1. Better user experience.
  2. Faster docs build because this results in about 270 fewer pages. From 4:30 to 4:10. This benefit will be much bigger when switching to the new Furo theme because it works around the O(n^2) behavior of the left side bar, where n is # of HTML pages, as described in Furo: investigate slow builds qiskit_sphinx_theme#328.

This does not migrate the visualizations module, though, since those functions tend to have huge docstrings with visuals included.

Some of our functions are documented on module pages but come from submodules, e.g. qiskit.circuit.random.random_circuit being documented on qiskit.circuit. To make it clear that you cannot directly import qiskit.circuit.random_circuit, this PR also changes our config to show the module path in code objects:

Screenshot 2023-07-26 at 6 22 59 AM

Follow up PRs should consider re-exporting those submodule members in the top-level module so that we can revert this config.

@Eric-Arellano Eric-Arellano added documentation Something is not clear or an error documentation stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: None Do not include in changelog labels Jul 21, 2023
@coveralls
Copy link

coveralls commented Jul 21, 2023

Pull Request Test Coverage Report for Build 5679422072

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 111 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-0.02%) to 85.914%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/library/n_local/efficient_su2.py 1 94.74%
qiskit/extensions/unitary.py 1 93.75%
crates/qasm2/src/lex.rs 4 90.38%
qiskit/circuit/library/n_local/two_local.py 5 87.8%
qiskit/circuit/library/n_local/qaoa_ansatz.py 9 88.89%
qiskit/transpiler/passes/routing/stochastic_swap.py 9 95.67%
qiskit/circuit/library/evolved_operator_ansatz.py 11 85.95%
crates/qasm2/src/parse.rs 18 96.67%
qiskit/circuit/library/n_local/n_local.py 53 84.65%
Totals Coverage Status
Change from base Build 5657291250: -0.02%
Covered Lines: 73046
Relevant Lines: 85022

💛 - Coveralls

@Eric-Arellano Eric-Arellano changed the title [wip] Document functions on module pages Document functions on module pages Jul 25, 2023
@Eric-Arellano Eric-Arellano marked this pull request as ready for review July 25, 2023 20:33
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @enavarro51
  • @Cryoris
  • @ElePT
  • @Qiskit/terra-core
  • @ajavadia
  • @ikkoham
  • @mtreinish
  • @nkanazawa1989
  • @woodsp-ibm

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks Eric, this looks like a solid start. I think we'll want to do more passes in the future improving the stories on these documentation pages, but that's definitely for another day. I've found that putting more of these methods on the same page seems to exacerbate CSS problems in the Pytorch theme, but hopefully that'll all be fixed by Furo anyway.

There's a few places where I think we need to be careful about .. currentmodule handling that autosummary would previously paper over, but mostly this looks fine to me.

qiskit/circuit/__init__.py Outdated Show resolved Hide resolved
qiskit/circuit/library/__init__.py Outdated Show resolved Hide resolved
qiskit/quantum_info/__init__.py Show resolved Hide resolved
qiskit/scheduler/__init__.py Outdated Show resolved Hide resolved
qiskit/scheduler/methods/__init__.py Show resolved Hide resolved
qiskit/transpiler/preset_passmanagers/__init__.py Outdated Show resolved Hide resolved
qiskit/utils/__init__.py Show resolved Hide resolved
Copy link
Collaborator Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks, Jake! You're spot on.

qiskit/quantum_info/__init__.py Show resolved Hide resolved
qiskit/scheduler/__init__.py Outdated Show resolved Hide resolved
@Eric-Arellano
Copy link
Collaborator Author

Huh..the last commit resulted in a maximum recursion error 😱 But @jakelishman I suspect it wouldn't solve the UX problem either way -- I think the functions would still only show the function name, rather than the full or relative module, so it would make the import path misleading.

--

Alternative idea. What if we stopped using .. currentmodule:: in general? That way our docs show the full module path, like this:

image

Imo, that is a better user experience because it allows you to immediately see how to import the code object. That's especially useful on long pages where you forget what module you're on.

Yes, it's more verbose in our __init__.py, but that doesn't seem like a big deal.

I would split that out as a dedicated "prefactor" PR.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

The UI of what is displayed is a bit of a separate question to the currentmodule directive, which tells Sphinx what module the contained objects should be considered as part of. If you want all the module names shown, then we should set add_modules_names=True to the root conf.py. I'd worry a little about that being very noisy for individual functions, but it could be ok, and it's not my call anyway.

For functions where it might not be clear how they're imported from the docs structure, I think that probably more indicates that the current import structures in Qiskit / incomplete explanations in the documentation, tbh. qiskit.circuit.random.random_circuit probably should just be available at qiskit.circuit.random_circuit.

qiskit/transpiler/preset_passmanagers/__init__.py Outdated Show resolved Hide resolved
@Eric-Arellano Eric-Arellano added this to the 0.25.0 milestone Jul 27, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I flicked through all the built documentation pages, and this looks like a good improvement to me. There's of course places where our styling could be improved, but we're hoping the switch to Furo will improve a lot of that immediately, and make the rest rather a lot easier to improve.

I pushed up a minor change to add some more autofunction calls in a couple of files that I spotted them in.

@jakelishman jakelishman enabled auto-merge July 27, 2023 10:28
@jakelishman jakelishman added this pull request to the merge queue Jul 27, 2023
@jakelishman jakelishman mentioned this pull request Jul 27, 2023
7 tasks
Merged via the queue into Qiskit:main with commit 025320f Jul 27, 2023
mergify bot pushed a commit that referenced this pull request Jul 27, 2023
* Document functions on module pages

* Fix invalid functions

* Revert visualization because each function is so big

* Fix more issues

* Use currentmodule

* Show modules & fix recursion

* Add some missing autofunctions

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
(cherry picked from commit 025320f)
@Eric-Arellano Eric-Arellano deleted the no-function-pages branch July 27, 2023 12:54
github-merge-queue bot pushed a commit that referenced this pull request Jul 27, 2023
* Document functions on module pages

* Fix invalid functions

* Revert visualization because each function is so big

* Fix more issues

* Use currentmodule

* Show modules & fix recursion

* Add some missing autofunctions

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
(cherry picked from commit 025320f)

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog documentation Something is not clear or an error documentation stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants