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.

Sign up for GitHub

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

Jump to bottom

Improve header hierarchy for utils documentation #11923

Merged
merged 7 commits into from
Mar 1, 2024
Merged

Improve header hierarchy for utils documentation #11923

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0df0d06
Improve header hierarchy for utils documentation
Eric-Arellano Feb 29, 2024
023069d
Fix pylint
Eric-Arellano Feb 29, 2024
a637306
Merge branch 'main' of github.com:Qiskit/qiskit-terra into EA/improve…
Eric-Arellano Feb 29, 2024
608d33c
Review feedback
Eric-Arellano Feb 29, 2024
7df1d87
Merge branch 'main' of github.com:Qiskit/qiskit-terra into EA/improve…
Eric-Arellano Feb 29, 2024
e530b31
Use mod cross-reference to self
Eric-Arellano Feb 29, 2024
3bcdc24
Merge branch 'main' into EA/improve-utils-headers
Eric-Arellano Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions qiskit/utils/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,42 @@

.. currentmodule:: qiskit.utils

Deprecations
------------

.. autofunction:: add_deprecation_to_docstring
.. autofunction:: deprecate_arg
.. autofunction:: deprecate_arguments
.. autofunction:: deprecate_func
.. autofunction:: deprecate_function
.. autofunction:: local_hardware_info
.. autofunction:: is_main_process

SI unit conversion
------------------

.. autofunction:: apply_prefix
.. autofunction:: detach_prefix

Class tools
-----------

.. autofunction:: wrap_method

Multiprocessing
---------------

.. autofunction:: local_hardware_info
.. autofunction:: is_main_process

Parallel Routines
-----------------
Copy link
Member

Choose a reason for hiding this comment

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

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.


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`)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Screenshot 2024-02-29 at 11 04 43 AM

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Screenshot 2024-02-29 at 12 41 46 PM

I'll make your tweaks though.

Copy link
Member

Choose a reason for hiding this comment

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

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.

============================================================
Optional Dependency Checkers
----------------------------
Copy link
Member

Choose a reason for hiding this comment

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

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.


.. automodule:: qiskit.utils.optionals
"""
Expand Down
6 changes: 3 additions & 3 deletions qiskit/utils/optionals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"""
.. currentmodule:: qiskit.utils.optionals

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.)

Copy link
Collaborator Author

@Eric-Arellano Eric-Arellano Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)).

be used to test if certain functionality is available, and optionally raise
:class:`.MissingOptionalLibraryError` if the functionality is not available.

Expand Down Expand Up @@ -52,7 +52,7 @@
:widths: 25 75

* - .. py:data:: HAS_CONSTRAINT
- `python-constraint <https://github.com/python-constraint/python-constraint>__ is a
- `python-constraint <https://github.com/python-constraint/python-constraint>`__ is a
constraint satisfaction problem solver, used in the :class:`~.CSPLayout` transpiler pass.

* - .. py:data:: HAS_CPLEX
Expand Down
Loading