-
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 equivalence-library rules between rzz
and cp
#13019
Conversation
One or more of the following people are relevant to this code:
|
These gates are locally equivalence (as are all the Ising-interaction gates), and this simple additional rule lets things like QFT, which are defined by Qiskit's default constructor in terms of `cp`, get converted into `rzz` or `rzx`. One `ControlledGate` test needed a case removing, because the underlying control mechanism now works correctly.
4b6b203
to
9a2add5
Compare
Pull Request Test Coverage Report for Build 10577197425Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
@@ -1438,7 +1438,6 @@ def test_control_zero_operand_gate(self, num_ctrl_qubits): | |||
@data( | |||
RXGate, | |||
RYGate, | |||
RZGate, |
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.
why did you need to remove this test?
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.
The test is asserting that a failure occurs, and for most of the gates in the list, they shouldn't really fail - we've got enough equivalence and translation rules that it should be possible to translate the gate into one we can represent better. The add_control
logic is just not super smart about gate translation.
As a side effect of this PR, the RZ path now happens to convert things to CPhaseGate
rather than CRZGate
(or something like that), so the control works further than it did before.
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.
LGTM. My only comment is that some equivalences related to rotation gates do not have an image, but this is not directly related to this PR, and can be added in another PR.
features_circuits: | ||
- | | ||
New equivalence rules in the standard equivalence library mean that the transpiler can now | ||
directly convert between two-qubit continuous Pauli rotations, rather than always decomposing |
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.
Did it always decompose into CX? There are some existing 2q Pauli equivalences that should allow to convert between some two-qubit rotations, no? 🙂
directly convert between two-qubit continuous Pauli rotations, rather than always decomposing | |
directly convert between two-qubit continuous Pauli rotations, rather than sometimes decomposing |
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'll tweak this note - I wrote it under the same false assumption about how the basis translator works that's alluded to in the PR comment. We need a new translation algorithm to make any choice of translation reliable - the current system is good at finding an arbitrary valid translation, but not at finding the best translation (for any definition of "best").
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've written a new version of the release note in bbe529b.
Summary
These gates are locally equivalence (as are all the Ising-interaction gates), and this simple additional rule lets things like QFT, which are defined by Qiskit's default constructor in terms of
cp
, get converted intorzz
orrzx
.Details and comments
I also have a complete rewrite of the equivalence library that is complete and in theory optimal (in 2q gate count) for translations at jakelishman/qiskit-terra@6654059193, but the current
BasisTranslator
algorithm does not work as well with the structuring of the rules. I have a new algorithm for translation in mind that I'm calling theBasisConstructor
, which will solve that problem, and also make basis translation for overcomplete basis sets and heterogeneous architectures more accurate, reliable and performant (hopefully!), but I haven't had chance to write it up yet.