-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 drawer support for AnnotatedOperations #11202
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7316922398
💛 - Coveralls |
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.
Many thanks @enavarro51, this looks amazing! And sorry for the late response.
The only small problem is that there could be multiple control modifiers in the list, for example we may have
op1 = AnnotatedOperation(SGate(), [InverseModifier(), ControlModifier(2, 1), ControlModifier(2)])
qc.append(op1, [0, 1, 2, 3, 4])
This can be easily fixed by combining multiple control modifiers into one and displaying that (please see one of the review comments for detail).
Technically we would not be able to see the number of control modifiers or understand which of the control qubits belong to which modifier, but I think it's completely adequate for displaying purposes. To be pedantic, even when we have a single control modifier and some other modifier, say inverse modifier, we can't know from the picture whether the inverse modifier is applied first and the control modifier is applied second, or the other way around, but it does not really matter since both are semantically equivalent, and again I feel it's completely adequate for displaying purposes.
qiskit/visualization/circuit/text.py
Outdated
# AnnotatedOperation with ControlModifier | ||
mod_control = None | ||
if getattr(op, "modifiers", None): | ||
for modifier in op.modifiers: | ||
if isinstance(modifier, ControlModifier): | ||
mod_control = modifier | ||
break | ||
|
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 would not work correctly when there are multiple ControlModifier
s in the list. The simplest fix would be to use the function _canonicalize_modifiers
from annotated_operation.py
which would combine multiple control modifiers into one, as follows:
if getattr(op, "modifiers", None):
canonical_modifiers = _canonicalize_modifiers(op.modifiers)
for modifier in canonical_modifiers:
if isinstance(modifier, ControlModifier):
mod_control = modifier
break
# AnnotatedOperation with ControlModifier | ||
mod_control = None | ||
if getattr(op, "modifiers", None): | ||
for modifier in op.modifiers: | ||
if isinstance(modifier, ControlModifier): | ||
mod_control = modifier | ||
break | ||
|
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.
Please see the comment in text.py
.
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.
Got it. I understand that displays are sometimes 'best efforts'. Changes done in 0c79f18.
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.
Many thanks for the quick updates!
Summary
Fixes #11087
Details and comments
This PR adds support for displaying the details of an
AnnotatedOperation
in the circuit drawers. If theControlModifier
is used, it displays as a controlled gate would. IfInverseModifier
orPowerModifier
is used, this is appended to the name of thebase_op
. Thepower
value is rounded to 1 decimal.This also fixes a bug in the 'text' drawer that would cause it to fail if an
AnnotatedOperation
was in the circuit.To do: