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

Docs (adapted from Qulacs) #14

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Docs (adapted from Qulacs) #14

merged 4 commits into from
Jun 20, 2024

Conversation

WrathfulSpatula
Copy link
Collaborator

@nathanshammah This is already fully adapted and working from the Qulacs repository. Really, users don't specifically need anymore information than this. (Does this look alright?)

@nathanshammah
Copy link
Member

LGTM. Note that this and #13 are basically duplicate, modulo the correct explanations in simulator.rst and toml file in this one, and the readme edits I made to include the invisible markers to have the installation and support menu display, see here. Either way is fine for me.

Note that automodapi in code.rstcaptures the init comment from pennylane_qrack, but not the qrack device module nor the docstrings or the QrackDevice class docstrings (maybe the docstrings are not formatted correctly, but not sure whether the qrack_device.py should show up in the API doc; changing toc tree to 2 or 3 did not help for me locally).

Copy link
Contributor

@cosenal cosenal left a comment

Choose a reason for hiding this comment

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

We should add a github workflow step to build the docs.

doc/devices/simulator.rst Outdated Show resolved Hide resolved
doc/installation.rst Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

To be updated after #15

Copy link
Member

@nathanshammah nathanshammah left a comment

Choose a reason for hiding this comment

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

@WrathfulSpatula looks good, just remember to add the markers to the main README.rst file like done in #13.

Note that the functions docstrings are not listed in the API doc, but the plugin is private. In any case, in another issue, we could at least format the QrackDevice class docstring, so that it shows up on the api page.

@nathanshammah
Copy link
Member

@WrathfulSpatula any idea why tests are failing? Should they pass?

@WrathfulSpatula
Copy link
Collaborator Author

@WrathfulSpatula any idea why tests are failing? Should they pass?

@nathanshammah They're not expected to pass until Catalyst v0.7 is released. I attempted to replace the test runner dependency with building from source, but Catalyst takes entirely too long to compile: the tests don't even complete, in that case. The tests work locally with v0.7, and they will pass once it's officially released to PyPi.

@WrathfulSpatula
Copy link
Collaborator Author

Note that the functions docstrings are not listed in the API doc, but the plugin is private. In any case, in another issue, we could at least format the QrackDevice class docstring, so that it shows up on the api page.

@nathanshammah, actually, we can't. While we can change the single top-level comment, in order to display docs for QrackSimulator, the class would need to be exported as a public API component. However, it shouldn't be exported publicly, because users never directly instantiate our classes or call any of our methods; this is already correct. We can alter that single string in the docs, or we could drop the API reference section, but users simply don't need to know anything about the API of QrackSimulator, by design of PennyLane.

Copy link
Member

@nathanshammah nathanshammah left a comment

Choose a reason for hiding this comment

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

LGTM!
Shall we open a separate issue to address @cosenal point on adding a "github workflow step to build the docs"? If it's easy to grab it from qulacs, let's do it in this PR, otherwise I'd move forward with merging this. We should also host the docs on RTD (other issue).

@cosenal cosenal self-requested a review June 20, 2024 15:30
Copy link
Contributor

@cosenal cosenal left a comment

Choose a reason for hiding this comment

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

LGTM

@WrathfulSpatula WrathfulSpatula merged commit f942b2f into master Jun 20, 2024
1 of 4 checks passed
@WrathfulSpatula WrathfulSpatula deleted the docs branch June 20, 2024 17:09
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