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 LaTeX drawer on split-filesystem systems #8629

Merged
merged 4 commits into from
Aug 29, 2022
Merged

Conversation

jmcelroy01
Copy link
Contributor

@jmcelroy01 jmcelroy01 commented Aug 29, 2022

Summary

Corrects issue #8542

  • I have added the tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 29, 2022
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@jmcelroy01
Copy link
Contributor Author

jmcelroy01 commented Aug 29, 2022

I'm unsure how to update a docstring (no. 2 in the pull request checklist).

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.

The CI is just failing because of the code-style issue. If you move the import to the top block (between import os and import subprocess) it should be happy again.

It would be good to add a release note to document the bug fix as well, but that will generally involve you setting up a proper development environment for Terra (see our contributing guide), but if you've not got time, let me know and I can take care of it for you.

@@ -488,7 +488,8 @@ def _latex_circuit_drawer(
image = utils._trim(image)
if filename:
if filename.endswith(".pdf"):
os.rename(base + ".pdf", filename)
import shutil
Copy link
Member

Choose a reason for hiding this comment

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

The import can go to the top of the file with the rest (I was just trying simplify the description of the change when I was asking you to try it out, sorry).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, good point. I moved it.

@jakelishman jakelishman changed the title Update circuit_visualization.py Fix LaTeX drawer on split-filesystem systems Aug 29, 2022
@jakelishman jakelishman added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Aug 29, 2022
@jakelishman
Copy link
Member

Oh, I missed your comment before - don't worry about the docstring or adding additional tests for this change. There's no documentation change necessary at all, and there's not really a sensible way we can test the LaTeX compilation on a split filesystem in CI anyway.

@jmcelroy01
Copy link
Contributor Author

It would be good to add a release note to document the bug fix as well, but that will generally involve you setting up a proper development environment for Terra (see our contributing guide), but if you've not got time, let me know and I can take care of it for you.

Yeah, please. I did the commits on Github directly. Will look into doing this for next time 😄

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.

Looks good, thanks for the debugging and the quick responses!

@jakelishman jakelishman added stable backport potential The bug might be minimal and/or import enough to be port to stable automerge labels Aug 29, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2949826278

  • 1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 84.129%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/circuit_visualization.py 1 2 50.0%
Totals Coverage Status
Change from base Build 2949248762: 0.003%
Covered Lines: 56835
Relevant Lines: 67557

💛 - Coveralls

@mergify mergify bot merged commit d163e89 into Qiskit:main Aug 29, 2022
mergify bot pushed a commit that referenced this pull request Aug 29, 2022
* Update circuit_visualization.py

Corrects issue #8542

* Update circuit_visualization.py

* Add release note

Co-authored-by: Jake Lishman <[email protected]>
(cherry picked from commit d163e89)
@jakelishman jakelishman linked an issue Aug 29, 2022 that may be closed by this pull request
mergify bot added a commit that referenced this pull request Aug 29, 2022
* Update circuit_visualization.py

Corrects issue #8542

* Update circuit_visualization.py

* Add release note

Co-authored-by: Jake Lishman <[email protected]>
(cherry picked from commit d163e89)

Co-authored-by: Joseph McElroy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QuantumCircuit.draw() invalid cross-device link error on linux
4 participants