Improve header hierarchy for utils documentation #11923
Conversation
|
One or more of the the following people are requested to review this:
|
| A helper function for calling a custom function with python | ||
| :class:`~concurrent.futures.ProcessPoolExecutor`. Tasks can be executed in parallel using this function. | ||
|
|
||
| .. autofunction:: parallel_map | ||
|
|
||
| Optional Dependency Checkers (:mod:`qiskit.utils.optionals`) |
There was a problem hiding this comment.
The code snippet makes this header unnecessarily long in the page table of contents. The table will already show the full import path, and I also tweaked the opening paragraph below to say the module name.
There was a problem hiding this comment.
A potential alternative is to give qiskit.utils.optionals its own rst file and entry in the toc, so it appears as a separate page? Our other module headers (that are standalone pages) all include this :mod:`qiskit.module` but in the title, so it's good for consistency.
That said, I don't feel strongly, and if this does stay in line, I'm fine to remove the bit from the title. It was originally included for consistency, and because I think the theme at the time didn't document things with fully qualified names.
There was a problem hiding this comment.
For it to be its own page in IQP, it will either need to be at the same level as utils, i.e. top-level like qiskit.utils.optionals in the left side bar; or qiskit.utils becomes like a folder that you have to click to expand and it has an Overview page and second page for optionals. Neither of those are very appealing. So let's stick with inline.
I'll make your tweaks though.
There was a problem hiding this comment.
Yeah, I'm fine either inline or separate page. I think I originally made this documentation, and I went inline in the original form too.
Pull Request Test Coverage Report for Build 8111735140Details
💛 - Coveralls |
qiskit/utils/optionals.py
Outdated
| Qiskit has several features that are enabled only | ||
| if certain *optional* dependencies are satisfied. This module is a collection of objects that can | ||
| Qiskit has several features that are enabled only if certain *optional* dependencies | ||
| are satisfied. The ``qiskit.utils.optionals`` module has a collection of objects that can |
There was a problem hiding this comment.
| are satisfied. The ``qiskit.utils.optionals`` module has a collection of objects that can | |
| are satisfied. This module, :mod:`qiskit.utils.optionals`, has a collection of objects that can |
or something? - I'm not tied to the wording, I just want to make it clearer that this section specifically documents one module, and so the module name is a hyperlink anchor to the same place to drive the point home.
(This comment is predicated on the :mod:`qiskit.utils.optionals` link being removed from the title - if that's reinstated, I think the original wording of the docs are probably fine.)
There was a problem hiding this comment.
and so the module name is a hyperlink anchor to the same place to drive the point home.
I thought this was confusing? I can add it back, but it seemed weird to me to click a link and it take you back to the exact spot you're reading.
The header will alreadyhave the little button to show it can be an anchor.
There was a problem hiding this comment.
If nothing else, not having the :mod: will mean that it gets styled very differently to most things documented objects - it's probably best to have it even if it's not all that useful. We have loads of examples of classes referring to themselves already, and all the :mod: tags that appear in titles will also be hyperlinking to themselves.
(Personally I don't find it confusing - I quite like it when things like the Python docs do it (e.g. https://docs.python.org/3/library/unittest.html)).
| A helper function for calling a custom function with python | ||
| :class:`~concurrent.futures.ProcessPoolExecutor`. Tasks can be executed in parallel using this function. | ||
|
|
||
| .. autofunction:: parallel_map | ||
|
|
||
| Optional Dependency Checkers (:mod:`qiskit.utils.optionals`) |
There was a problem hiding this comment.
A potential alternative is to give qiskit.utils.optionals its own rst file and entry in the toc, so it appears as a separate page? Our other module headers (that are standalone pages) all include this :mod:`qiskit.module` but in the title, so it's good for consistency.
That said, I don't feel strongly, and if this does stay in line, I'm fine to remove the bit from the title. It was originally included for consistency, and because I think the theme at the time didn't document things with fully qualified names.
qiskit/utils/__init__.py
Outdated
| Multiprocessing | ||
| --------------- | ||
|
|
||
| .. autofunction:: local_hardware_info | ||
| .. autofunction:: is_main_process | ||
|
|
||
| Parallel Routines | ||
| ----------------- |
There was a problem hiding this comment.
Can maybe stick these "Multiprocessing" functions under "Parallel Routines" with parallel_map, or vice versa? I think they're all sufficiently related to avoid an extra section heading.
qiskit/utils/__init__.py
Outdated
| ============================================================ | ||
| Optional Dependency Checkers | ||
| ---------------------------- |
There was a problem hiding this comment.
To stop this mistake happening again, we maybe want to make all the level-2 headings ===== underlines rather than ----- - that's the standard convention in Qiskit docstrings, and possibly how this mistake happened in the first place.
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
|
Oh dear, that test error is #11676, except it just happened on Python 3.8 / Numpy 1.24.4, which we thought couldn't happen. |
There was a problem hiding this comment.
This looks good to me, thanks for the changes.
Just for clarity if anyone else sees this - rST doesn't actually prescribe the syntax for h1/h2/h3 or anything, it just defines something like 12 ways you can make a header, then it infers which one refers to which level by the order they appear. We switched to ====== underlines here "for h2" not because that enforces h2, but just because conventionally in Qiskit documentation that's the one we use for the second heading level in a file.
|
Thanks, Jake. FYI I opened up Qiskit/documentation#942 to track auditing all the other module pages. |
* Improve header hierarchy for utils documentation * Fix pylint * Review feedback Co-authored-by: Jake Lishman <jake.lishman@ibm.com> * Use mod cross-reference to self --------- Co-authored-by: Jake Lishman <jake.lishman@ibm.com> (cherry picked from commit 44e348a)
* Improve header hierarchy for utils documentation * Fix pylint * Review feedback Co-authored-by: Jake Lishman <jake.lishman@ibm.com> * Use mod cross-reference to self --------- Co-authored-by: Jake Lishman <jake.lishman@ibm.com> (cherry picked from commit 44e348a) Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
* Improve header hierarchy for utils documentation * Fix pylint * Review feedback Co-authored-by: Jake Lishman <jake.lishman@ibm.com> * Use mod cross-reference to self --------- Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Groups functions into sections
This provides better structure for users rather than being a flat list. Each section header corresponds to a specific module.
Using these h2 headers also improves the accessibility of the docs with IQP. In qiskit/documentation, functions have an
h3generated for them like this:We generate this h3 so that
a) it shows up in the right page table of contents, and
b) the app automatically generates an anchor link buton
But accessibility guidelines say that you should avoid jumping from an h1 to h3 because it makes screen readers work worse.
Fixes
Optional Dependency Checkersto be h2It was being set as an h3 because of using
===as the header rather than---. Note that Sphinx doesn't have any predetermined heading value for===vs---. It determines what it corresponds to based on what shows up first in the document. Here,---shows up first, so it was an h2, whereas===was an h3.This issue was causing all subheaders to be one heading level lower than they should be.