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
New OptimizeAnnotated transpiler pass #11476
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7607475432Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - 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 LGTM, it's a straightforward pass to canonicalize any annotated operations and a good start to optimizing things. I left a few inline comments on some small things.
The higher level question is where were you thinking of adding this in the transpilation pipeline. I was thinking in the init
stage probably before before we run high level synthesis. We can do that here or in a follow up PR whichever you think would be better.
(instructions in this library will not be optimized by this pass). | ||
basis_gates: Optional, target basis names to unroll to, e.g. `['u3', 'cx']` | ||
(instructions in this list will not be optimized by this pass). | ||
Ignored if ``target`` is also specified. |
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.
Do you think having an explicit option to recurse into definitions is useful or not? Right now it's only done implicitly based on the presence of a Target
or basis gates. But do you think there is value in having an override for it?
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.
After thinking about this, such an option could definitely be useful and is added in fb7e279.
The current logic is the following (please see if you agree with this):
- If neither
basis_gates
nortarget
is specified, then we do not recursively descent into gate definitions, regardless of therecurse
option. This is consistent with the behavior ofHighLevelSynthesis
pass (and previously ofUnrollCustomDefinitions
pass) which in this case does not descend into the definitions either, i.e. we shouldn't optimize what we are not going to synthesize. - If either
basis_gates
ortarget
is specified, andrecurse
has the default value ofTrue
, then we do the recursion. - But we can override the recursion by setting
recurse
toFalse
.
This also ties in to the question of where this pass should sit in the transpilation pipeline. I was also thinking about the init
stage (just before running high level synthesis). Alternatively, if you think that having yet another recursive pass would be slow, we could use it from within HighLevelSynthesis
itself, i.e. when HighLevelSynthesis
needs to synthesize a DAGCircuit
(possibly as part of its own recursion), it can first call OptimizeAnnotated
on this dag circuit with recurse=False
. What do you think?
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, this logic makes sense to me. I think leaving this as a standalone pass makes sense, just as it seems like a slightly different building block in a pipeline that high level synthesis. However, I do agree there is probably some concern around performance if we're going to run with recurse=True
in the init
stage and I like your suggestion of calling it internally when it makes sense. I think when we investigate integrating this into the preset pass managers as part of 1.1.0 we can look at all those details.
Thanks! Please see this comment #11476 (comment) about thoughts on an explicit option to recurse into gate definitions and when in the transpiler flow the pass should sit. If you think that the best way to use this pass is in the |
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 LGTM now, thanks for the updates.
(instructions in this library will not be optimized by this pass). | ||
basis_gates: Optional, target basis names to unroll to, e.g. `['u3', 'cx']` | ||
(instructions in this list will not be optimized by this pass). | ||
Ignored if ``target`` is also specified. |
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, this logic makes sense to me. I think leaving this as a standalone pass makes sense, just as it seems like a slightly different building block in a pipeline that high level synthesis. However, I do agree there is probably some concern around performance if we're going to run with recurse=True
in the init
stage and I like your suggestion of calling it internally when it makes sense. I think when we investigate integrating this into the preset pass managers as part of 1.1.0 we can look at all those details.
I mentioned this in my inline response, but given the timing we can investigate how to integrate this pass into the preset pass managers for the 1.1 release. |
Summary
This PR adds a new
OptimizeAnnotated
transpiler pass that optimizes annotated operations on a quantum circuit, please refer to #11475. At the moment the pass only implements simple optimizations (bringing annotations into canonical form, removing annotations when possible, and combining multiple recursive annotations) and defines the API for the pass.Details and comments
Here is an example (also appearing in the release notes):
In the case of
gate1
, the modifiers of the annotated swap gate are brought into the canonical form: the twoInverseModifier
s cancel out, and the twoControlModifier
s are combined. In the case ofgate2
, all the modifiers get removed and the annotated operation is replaced by its base operation. In the case ofgate3
, multiple layers of annotations are combined into one.I am trying to find some balance between creating too many pull requests vs. making each pull request easier to review. My current plan is to add other optimizations (canceling inverse annotated operations and conjugate reduction) in a follow-up PR.