-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 a bug in QuantumCircuit.draw related to vertical_compression #9855
Fix a bug in QuantumCircuit.draw related to vertical_compression #9855
Conversation
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:
|
|
As far as I can see the two failing tests already fail on the main branch (and seem to be completely unrelated to what I did). I observed also a linter error in the file I updated but in line 647, far away from my change. I assume that the linter is run "lazily" in some way and some update on the linter side (in the past) triggered this now. |
fd0c74e
to
bdd7589
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this, and for the fix! Please could you add a one-line bugfix release note (reno new --edit fix-circuit-drawing-low-compression
, then delete all but the "fixes" bit and edit) saying that you've fixed the top-right corner of low-compression circuit visualisations?
I didn't happen to see what the test failures were, but given the time you opened the PR, it probably coincided with the time that symengine
version 0.10 had just released, which broke our CI temporarily.
Pull Request Test Coverage Report for Build 4597914946
💛 - Coveralls |
@jakelishman I added the release note. Thanks for the info on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super, thanks for this!
…kit#9855) * Fix a bug in QuantumCircuit.draw related to vertical_compression * Update tests so that they would catch the bug * Fix black-errors * Remove TODO comment * Add release note * Fixup release note --------- Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
…kit#9855) * Fix a bug in QuantumCircuit.draw related to vertical_compression * Update tests so that they would catch the bug * Fix black-errors * Remove TODO comment * Add release note * Fixup release note --------- Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Summary
When using
QuantumCircuit.draw('text')
there was a wrong character used for drawing boxes in case ofvertical_compression="low"
. I fixed this bug by a oneliner and updated the tests. I did not find a corresponding open issue.Details and comments
The bug is appears in this example:
The "high"-circuit is drawn correctly, but the second Hadamard of the seconed circuit is drawn incorrectly. It incorrectly uses the character
┤
for the upper right corner of the Box of the Gate.After the fix the output looks like this:
Btw: I observed that the argument
icod
of the methodmerge_lines
is somewhat "abused". Its docstring saysicod (top or bot): in case of doubt, which line should have priority? Default: "top".
. I am aware that I did not adhere to this statement. But My code change is consistent with other similar cases. See for example the case for the upper left corner which is treated in the same way. If somebody feels that this inconsistency is an issue I would propose to open an issue, but right now my change just fixes the bug in the simplest way possible without introducing more inconsistencies.Tests: I actually found another kind of bug in the tests. The test class
TestTextDrawerVerticalCompressionLow
certainly wants to test thevertical_compression="low"
feature. But two of those tests actually did not use low compression. I changed those to use low compression hence establishing two tests for my fix. I carefully checked that otherwise the output looks as before.