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

Sphinx documentation for the Python API #40

Merged
merged 43 commits into from
Jul 2, 2021
Merged

Sphinx documentation for the Python API #40

merged 43 commits into from
Jul 2, 2021

Conversation

Mandrenkov
Copy link
Collaborator

@Mandrenkov Mandrenkov commented Jun 29, 2021

Context:
Presently, the Jet documentation website only shows API documentation for the Jet C++ library despite the fact that the jet Python package is fully-featured and has additional functionality for modelling quantum circuits.

Description of the Change:

  • The "API" section of the Jet documentation website has been renamed to "C++ API".
  • The Sphinx build now generates documentation for the jet Python package under the "Python API" heading.
  • Many constructor docstrings have been upgraded to class docstrings so that they appear in the Sphinx summaries.

Benefits:

  • There is rendered API documentation for the jet Python package.

python_api_section

Possible Drawbacks:

  • The Sphinx build takes significantly longer to finish (still on the order of seconds but noticeably longer).

Related GitHub Issues:
None.

@Mandrenkov Mandrenkov added the documentation 📚 Improvements or additions to documentation label Jun 29, 2021
@Mandrenkov Mandrenkov requested review from josh146 and brownj85 June 29, 2021 19:11
@github-actions
Copy link

github-actions bot commented Jun 29, 2021

Test Report (C++) on Ubuntu

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
521 tests ±0  521 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
870 runs  ±0  870 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 29805e4. ± Comparison against base commit 29805e4.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jun 29, 2021

Test Report (C++) on MacOS

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
521 tests ±0  521 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
870 runs  ±0  870 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 29805e4. ± Comparison against base commit 29805e4.

♻️ This comment has been updated with latest results.

@Mandrenkov Mandrenkov marked this pull request as ready for review June 29, 2021 19:15
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Looks great @Mandrenkov! I downloaded and had a look at the rendered site, really impressive so far. Left some comments and thoughts, mostly minor.

I also had some other thoughts while reviewing which are out-of-scope, but just in case:

  • If you want the index.rst page to appear in the ToC, we found out later that this is possible since Sphinx supports links in the ToC directly e.g., see the PennyLane-Qiskit docs (the index page is listed as 'Overview' on the LHS). However, we never got around to propagating this elsewhere.

  • The 'Key Concepts' and 'Getting Started' quick links on index.rst are broken

  • Are the C++-Python pybind11 bindings documented anywhere? I clicked around but couldn't find them in either the C++ section or the Python API section 🤔 I thought Sphinx would be able to automatically extract them.

  • It would be good to offer minimal example usage within every user-facing docstring, a la NumPy. In particular, I found myself thinking this while looking at jet.run_xir_program. We find this important as:

    • over time, users will end up directly on the docstring page via a Google search, so it's the best place to provide usage information
    • the example will also be physically close to the actual code, making it easy to remember to update when making changes in PRs

.github/CHANGELOG.md Outdated Show resolved Hide resolved
docs/_templates/autosummary/base.rst Show resolved Hide resolved
docs/code/jet.rst Show resolved Hide resolved
docs/code/jet.rst Show resolved Hide resolved
docs/code/jet_circuit.rst Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
include/jet/Tensor.hpp Show resolved Hide resolved
python/jet/circuit.py Outdated Show resolved Hide resolved
python/jet/circuit.py Outdated Show resolved Hide resolved
@Mandrenkov
Copy link
Collaborator Author

Looks great @Mandrenkov! I downloaded and had a look at the rendered site, really impressive so far. Left some comments and thoughts, mostly minor.

Excellent comments! 💯

  • If you want the index.rst page to appear in the ToC, we found out later that this is possible since Sphinx supports links in the ToC directly e.g., see the PennyLane-Qiskit docs (the index page is listed as 'Overview' on the LHS). However, we never got around to propagating this elsewhere.

Ah, very nice! I played around with this but ultimately decided to keep the current layout since it matches PL and SF. Also, creating a separate ToC section for the landing page feels wasteful and having the overview under "Using Jet" looks out of place.

  • The 'Key Concepts' and 'Getting Started' quick links on index.rst are broken

Fixed. 😅

  • Are the C++-Python pybind11 bindings documented anywhere? I clicked around but couldn't find them in either the C++ section or the Python API section 🤔 I thought Sphinx would be able to automatically extract them.

I initially tried to generate API documentation for the Python bindings but couldn't figure out how to do it so I submitted the PR without them; however, after looking at it today, I was able to get it work so they should all be there now!

  • It would be good to offer minimal example usage within every user-facing docstring, a la NumPy.

Agreed! I added some examples to the docstrings dealing with the XIR but the others will have to wait for a future PR.

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Python bindings look 🤩

I didn't look through them exhaustively, but did notice a few indentation issues?

image

image

If this is not something fixable in the C++, though, one option could just be to tweak the CSS so that indentation/quotes aren't noticeable 😆

.github/workflows/documentation.yml Show resolved Hide resolved
.github/workflows/documentation.yml Show resolved Hide resolved
python/jet/interpreter.py Show resolved Hide resolved
python/jet/interpreter.py Show resolved Hide resolved
python/jet/interpreter.py Outdated Show resolved Hide resolved
@Mandrenkov
Copy link
Collaborator Author

I didn't look through them exhaustively, but did notice a few indentation issues?

Ah, yes. It turns out we need to remove the (prefixed) indentation from the Python binding docstrings. Furthermore, to better distinguish the different function overloads, I've put their signatures in bold and italics and separated them with divider lines.

Now, overloaded functions looks like this:

overloaded_function

Similarly, overloaded methods look like this:

overloaded_method

@josh146
Copy link
Member

josh146 commented Jul 2, 2021

@Mandrenkov looks great 🤩

@Mandrenkov Mandrenkov merged commit 29805e4 into main Jul 2, 2021
@Mandrenkov Mandrenkov deleted the python-api-docs branch July 2, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📚 Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants