-
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
Add barrier labels and display in 3 circuit drawers #8440
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:
|
Pull Request Test Coverage Report for Build 2890221517
💛 - Coveralls |
…ra into show_barrier_label
…ra into show_barrier_label
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.
This looks great, thanks Edwin. How well does it handle barriers that don't include the top-most qubit in each of the drawers? I had a quick play, and I think the text drawer adds an extra blank line which messes up subsequent spanning gates:
import qiskit
qc = qiskit.QuantumCircuit(6)
qc.barrier((0, 1), label="a")
qc.barrier((3, 4, 5), label="b")
qc.cx(1, 4)
qc.draw()
gives
a
q_0: ─░──────
░
q_1: ─░───■──
░ │
q_2: ─────┼──
│
b
q_3: ─░──────
░ ┌─┴─┐
q_4: ─░─┤ X ├
░ └───┘
q_5: ─░──────
░
I guess it's something to do with the layer joining logic? It seems to work fine in the mpl drawer (though a little nitpick: the font of the label is rather small).
The latex form overlapping the barrier isn't ideal, but then again, the barriers don't produce a particularly great effect in the latex drawer anyway. Let's not worry about it.
I'll dig into the text problem. It's repeatable. Replacing the barrier piece with text should not change the connections. (Edit) This relates to
So this is something that needs to be addressed for controlled gates with |
Also on the font size for 'mpl' drawer. There are 2 defined font sizes in the styles - Done in 1a17e25. |
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 the changes Edwin. I had one minor design question (first review comment), but other than that, this is good to merge. Thanks for finding the issue with vertical_compression
- it'd definitely be good to get that fixed if we're adding situations where it's going to be called implicitly within library code.
if node.op.label is not None: | ||
pos = indexes[0] | ||
label = node.op.label.replace(" ", "\\,") | ||
self._latex[pos][col] = "\\cds{0}{^{\\mathrm{%s}}}" % label |
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.
Perhaps it would make more sense to wrap the label in \text
instead of \mathrm
- the latter still uses math-mode spacing/kerning rules (hence the \,
), whereas \text
swaps back to the full text font. It depends on amsmath
being available (I think), but that's part of the de facto LaTeX standard library. Users would be able to have parts of the label rendered in math-mode by including the $ ... $
manually.
That's just on the basis that I would expect "label" to be a text field, not an equation one. If we feel like it's more of an "equation" field, then the current math-mode way makes sense.
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.
\text
doesn't work on my TeX Live version of LaTex. I don't think we'd want to do another usepackage
for amsmath
just for this, so I'd suggest we leave the \mathrm
as is.
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.
Yeah, that's fair enough. I'm surprised that nothing is already loading amsmath
, since it's about as common as import numpy
in mathematical Python code, but fair enough. If it's not already loaded, yeah, let's not add the extra dependency.
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 this, I can certainly see it as a useful feature for visualisation of complex circuits.
Summary
Fixes #8428
Fixes #1298
Details and comments
This PR adds a
label
parameter to theBarrier
class and displays the label at the top of the barrier in the 3 circuit drawers. The existing label onsnapshot
directives is now also displayed. The display in the 'latex' drawer may not be optimal, but there are limitations in qcircuit for displaying text outside boxes. The info is still readable and does not overwrite any gate info.