-
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
Update gate.inverse()
to return AnnotatedOperation
#11593
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7744292754
💛 - 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.
Overall this looks fine, it's a similar addition to #11433. I just have a few inline comments. The other higher level question is around the standard gates, most of them add the new kwarg to be compliant with the interface inherited from Instruction
, but they then ignore the value of the annotated
kwarg and always return the discrete definition of the inverse. You called this out in the PR summary, but I'm wondering if we still want to return return super().inverse(annotated=annotated)
if it's true even if there isn't much of a use case. I'm worried it might be confusing for users if they call CXGate().inverse(annotated=True
and just get a CXGate
back. Even though cx is a self inverse maybe the use case for annotated in that case is that they want to pair with other modifiers for some potential optimization in the transpiler. TBH, I realize that argument is a bit of a stretch I'm mostly just worried about api consistency with the new option
Also this is missing a release note documenting this new feature.
Thanks! I have added the release notes and a few tests (somehow I forgot to do this either). At the moment I can't think of a scenario where keeping an inverse I was actually thinking earlier that we may want to have 3 different values for
We need |
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.
I'm ok with that decision as long as we document it clearly. I started leaving inline suggestions to update the docs but then decided we can do that in a follow up PR after RC1. I think the only blocker on this right now is the CI failure which I think is coming from the type hint and sphinx not being able to resolve Instruction
correctly.
qiskit/circuit/instruction.py
Outdated
@@ -417,22 +419,34 @@ def reverse_ops(self): | |||
reverse_inst.definition = reversed_definition | |||
return reverse_inst | |||
|
|||
def inverse(self): | |||
def inverse(self, annotated: bool = False) -> Instruction | AnnotatedOperation: |
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.
It's unfortunate that there are 2 Instruction
classes in qiskit one for circuits and the other in pulse. The ci docs failure is because sphinx couldn't tell them apart with just the Instruction
class listed here. Sphinx doesn't really understand type hinting in the same manner that python does apparently, because in python there is enough context to know that Instruction
here is the class
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, I realized that before I saw your comment :).
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
…it-terra into inverse-to-annotated
Summary
This PR adds a new argument
annotated
to all the "inverse" methods. This is a follow-up to #11433 (and should be rebased onto that).Details and comments
Note that even though we have to update the interface of many standard gates that implement
inverse
, in practice inverses of standard gates are also standard gates, and we should always prefer these over an "inverse-annotated" operation. For instance, the inverse of theCX
-gate is again theCX
-gate, and the inverse of theS
-gate is theSdg
-gate (and in addition all of these are singleton gates).I believe that for the immediate release it makes sense to either include both this and #11433 together or to include neither. I have updated the priority to that of #11433.