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

Replace parsed-literal with code-block #13257

Merged
merged 12 commits into from
Oct 17, 2024

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented Oct 2, 2024

From @frankharkins:

I'd avoid parsed-literal and just go with .. code-block:: text for outputs. Parsed literal might not behave how you expect: https://docutils.sourceforge.io/docs/ref/rst/directives.html#parsed-literal-block

@jakelishman's response:

Yeah, I don’t actually know why we ever used parsed-literal. I assume there was a reason at some point, but just on the surface of it, I can’t imagine why we ever would have wanted to parse what’s in a text output for rst blocks

--

This change was generated with rg 'parsed-literal::' -l | xargs sd 'parsed-literal::' 'code-block:: text'.

This PR also fixes some circuit library docs that had module docstring that was not being built. Those docs are moved into their appropriate function docstrings.

@coveralls
Copy link

coveralls commented Oct 2, 2024

Pull Request Test Coverage Report for Build 11373330230

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 18 of 18 (100.0%) changed or added relevant lines in 9 files are covered.
  • 667 unchanged lines in 27 files lost coverage.
  • Overall coverage decreased (-0.2%) to 88.658%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/accelerate/src/elide_permutations.rs 1 97.06%
crates/accelerate/src/error_map.rs 1 85.42%
crates/accelerate/src/circuit_library/quantum_volume.rs 1 96.69%
qiskit/circuit/library/quantum_volume.py 2 95.45%
crates/accelerate/src/barrier_before_final_measurement.rs 2 97.33%
crates/circuit/src/lib.rs 2 96.15%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.19%
crates/circuit/src/bit_data.rs 2 94.62%
crates/accelerate/src/commutation_checker.rs 2 97.14%
Totals Coverage Status
Change from base Build 11241264267: -0.2%
Covered Lines: 73185
Relevant Lines: 82548

💛 - Coveralls

Copy link
Member

@frankharkins frankharkins left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Only nit is that maybe outputs should always be text, even when they happen to be valid Python? Lack of highlighting might help users distinguish code and outputs, especially when the two blocks are back-to-back. It's also the default behaviour of ipython and jupyter notebooks.

@Eric-Arellano Eric-Arellano changed the title [wip] Replace parsed-literal with code-block Replace parsed-literal with code-block Oct 3, 2024
@Eric-Arellano Eric-Arellano marked this pull request as ready for review October 7, 2024 13:08
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia
  • @nkanazawa1989

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 for doing this - code-block:: text feels much more intended than parsed-literal for us. I wonder what the initial reason was.

The only question is that you seem to have introduced newlines in lots of places, but there's a few missing. I'm assuming that means that the newlines are important, which if so, means that there's a few places with substantial docstrings that don't appear to be included in the build.

I'm fine merging the PR as-is if that is the case, but we should follow up on restructuring to ensure that all the identified places are built in if they're supposed to be.

@Eric-Arellano
Copy link
Collaborator Author

Ah, I added the newlines in a follow up commit because the Sphinx build was failing without them. But I only fixed the places Sphinx complained about: I didn't exhaustively go through every file.

Even though Sphinx ignores those places you flagged, I'll still fix them today. We might want those docs in the future, and it reduces the risk of someone being confused when they copy-and-paste the code for new docs and it fails.

@jakelishman
Copy link
Member

Ah cool, thanks. I suspect that the base_scheduler one is truly indicative of a problem - those docs look substantial, and we didn't catch the problem in the build, which I'm guessing means they're not being built in, but probably should be. We can address that bit (adding it to the build somewhere) in a follow up.

@Eric-Arellano
Copy link
Collaborator Author

@jakelishman how would you like me to fix the missing module docstring lint? Those aren't used by Sphinx, so it seems like a risk people will write good docs that end up getting ignored. Perhaps it's best to disable the lint?

@jakelishman
Copy link
Member

For the time being, I guess just put in a dummy one-line docstring or a # pylint: disable=missing-module-docstring suppression - it won't be the first nor the last in Qiskit.

Longer term, yeah, we should think about whether the module lint is truly adding anything - I suspect not, because most modules in Qiskit aren't public, so their docstrings aren't used. The class and function docstring lints are probably more sensible (though it'd be better if we could blanket suppress them in tests).

@Eric-Arellano
Copy link
Collaborator Author

Bump

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 for doing this. I really don't know why the template circuits are organised like that - if we ever manage to finish overhauling the template-optimisation engine, we definitely should tidy that up.

@jakelishman jakelishman enabled auto-merge October 16, 2024 20:27
@jakelishman jakelishman added documentation Something is not clear or an error documentation Changelog: None Do not include in changelog labels Oct 16, 2024
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.

Whoops, forgot to actually leave the tick.

@jakelishman jakelishman added this pull request to the merge queue Oct 17, 2024
Merged via the queue into Qiskit:main with commit 68a1eca Oct 17, 2024
15 checks passed
@Eric-Arellano Eric-Arellano deleted the EA/bye-bye-parsed-literal branch October 17, 2024 12:24
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants