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

Fix two latex issues in docs #13388

Merged
merged 2 commits into from
Nov 1, 2024
Merged

Conversation

Eric-Arellano
Copy link
Collaborator

These cause the docs app to crash.

@Eric-Arellano Eric-Arellano added documentation Something is not clear or an error documentation Changelog: None Do not include in changelog labels Nov 1, 2024
@Eric-Arellano Eric-Arellano requested a review from a team as a code owner November 1, 2024 19:21
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@@ -33,7 +33,7 @@ class RemoveIdentityEquivalent(TransformationPass):

.. math::

\bar{F} = \frac{1 + F_{\text{process}}{1 + d}
\bar{F} = \frac{1 + F_{\text{process}}}{1 + d},
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 comma is only to make the two equations render more nicely.
Screenshot 2024-11-01 at 3 22 29 PM

Copy link
Member

@jakelishman jakelishman Nov 1, 2024

Choose a reason for hiding this comment

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

You might want to put an enforced space after the comma to improve it - probably one of \ or \quad depending on how much you want. I'm fine to just merge if this is already good enough for you.

jakelishman
jakelishman previously approved these changes Nov 1, 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.

Thanks for this. Any chance you know a simple way to get Sphinx to attempt the renders server side, so we can put the checks in our CI?

For my own website, I run KaTeX deployment-side as part of the page-build operation, but since we're not already running any Node-based stuff here, it'd be a bit of a faff if we had to do that via Node, especially for local contributors.

@Eric-Arellano
Copy link
Collaborator Author

Eric-Arellano commented Nov 1, 2024

Any chance you know a simple way to get Sphinx to attempt the renders server side, so we can put the checks in our CI?

I don't know of anything off the top of my head, but I added that we should investigate this as part of Qiskit/documentation#2228. I know Kevin Sung had switched Qiskit to Katex with Sphinx, so I'm surprised the Sphinx build pass - I have not yet tried triaging what went wrong. I only just found the issue to begin with.

For what it's worth, qiskit/documentation should have caught this issue much sooner. We have a check that every changed page renders, but it didn't correctly run. That's tracked by Qiskit/documentation#2228. So, I didn't notice the issue until doing an open-source sync in the closed source repository. We recently got the nightly dev docs job working, so qiskit/documentation in theory will catch Latex issues every night. That might be sufficient.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11635261948

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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 36 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.004%) to 88.721%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/two_qubit_decompose.rs 1 92.09%
crates/qasm2/src/lex.rs 8 91.98%
crates/circuit/src/circuit_data.rs 27 95.55%
Totals Coverage Status
Change from base Build 11634403304: 0.004%
Covered Lines: 76366
Relevant Lines: 86074

💛 - Coveralls

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.

KaTeX is also client-side JS-based rendering by default like MathJax, and our Sphinx output is certainly set up for client-side rendering, which is why it's not getting caught here.

Looks like sphinxcontrib-katex does have a way of doing the pre-rendering if Node is installed, so maybe there's a sensible path through. I wouldn't want to do that as part of the actual build, though, since we don't want to be making the HTML rendering decisions in this repo.

@jakelishman jakelishman added this pull request to the merge queue Nov 1, 2024
Merged via the queue into Qiskit:main with commit 4c3f8c9 Nov 1, 2024
17 checks passed
@Eric-Arellano Eric-Arellano deleted the EA/fix-latex branch November 2, 2024 12:36
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.

4 participants