Improve single-qubit gate count in TwoQubitControlledUDecomposer#16123
Conversation
Coverage Report for CI Build 25690415793Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.05%) to 87.621%Details
Uncovered Changes
Coverage Regressions764 previously-covered lines in 18 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
One or more of the following people are relevant to this code:
|
TwoQubitControlledUDecomposerTwoQubitControlledUDecomposer
mtreinish
left a comment
There was a problem hiding this comment.
Overall this looks good, thanks for fixing this. I just had a few comments inline.
mtreinish
left a comment
There was a problem hiding this comment.
Thanks for the updates, I just had a few more small comments inline.
This commit updates the result type usec internally between the to_rxx_gate() function and it's caller to be a struct instead of a 4-tuple. This allows named access and makes it easier to reason about about what each of the fields are and give them context.
mtreinish
left a comment
There was a problem hiding this comment.
Overall this LGTM now and is basically ready to merge. Thanks for doing this. I did pushed up a small couple of commits to cleanup some of the code (using a struct instead of a tuple so we get named access on the return from to_rxx_gate() and also renaming a couple of variables. I won't automerge it so you can review that you're ok with my changes before we merge. If you're ok with them feel free to enqueue for merging.
I also had a comment on the tests which we would be good to fix now, but also it isn't critical since it's really more a test of the euler one qubit decomposer.
| self.assertLessEqual(circ.count_ops().get("u", 0), 8) | ||
| self.assertLessEqual(circ.count_ops().get("sx", 0), 14) | ||
| self.assertLessEqual(circ.count_ops().get("rx", 0), 14) | ||
| self.assertLessEqual(circ.count_ops().get("ry", 0), 8) | ||
| self.assertLessEqual(circ.count_ops().get("rz", 0), 22) |
There was a problem hiding this comment.
I think we can tighten this up by adding a condition on the basis. Like right now none of the tests really check the 1q gate count for the U3, U321, or RR basis. We should maybe validate the gate count for a given basis matches the specified euler basis or make the check more generic and say we expect < n single qubit gates where n is determined by the euler basis.
There was a problem hiding this comment.
Thanks for the fixes you've pushed - the code looks nicer now!
Since this test is quite complicated as it includes many combinations of 2-qubit gates and euler bases, this is just a sanity check (since we already have seperate tests for the euler decomposers).
The main check is that the maximal number of u gates is 8.
Similarly for ry gates as they appear only in ZYZ.
rx gates can appear only in ZXZ (at most 8 times), or XYX / XZX, so their number is at most 16.
The rz gate count is not tight (it's at most 8 in XZX, 16 at ZYZ and ZXZ and more in ZSX and ZSXX), but since they don't have an error, I don't care much about their counts for now.
This commit adds an option to the TwoQubitPeepholeOptimization transpiler pass added in Qiskit#13419 to allow configuring the heuristic priority when instantiating the pass. When the pass is making a decision on which potential synthesis outcome out of multiple is the best or when to replace a block with the best synthesis outcome there are three metrics we look at, the estimated fidelity, the number of 2q gates, and the total number of gates. The order of this comparison is inherently flexible and prioritizes different aspects of the synthesis. This exposes a new option to the pass to enable users to specify which aspect they want the pass to prioritize. Previously the pass was hard coded to prioritize 2q gate count, but as was discussed in the PR review and the commit history of the PR branch this isn't necessarily the ideal choice. Intuiatively assuming relatively accurate error rates in the target the estimated fidelity should be the first priority. This was changed to prioritize two qubit gate count during development as a debugging step and left in place while we worked on issues with the TwoQubitControlledUDecomposer's 1q component handling (see Qiskit#16123). Now that the TwoQubitControlledUDecomposer issue has been resolved we can explore switching the default, this new argument will make it much easier to do side by side comparisons of the different options. The intent is to enable updating the usage in the preset pass managers (added in Qiskit#16136) without having to modify the pass's rust code. This does not include a release not as the the new pass has not been included in a release yet so there is no addition to a public API in this PR.
close #16036
This PR reduces the total number of 1-qubit unitaries needed to synthesize a general 2-qubit unitary using
TwoQubitControlledUDecomposerfrom 24 to 8.This is in particular useful for the peephole optimization in #13419.
Details:
The
TwoQubitControlledUDecomposerconsists of the following steps.We start from a general 4x4 unitary U.
Then we find a cannonical gate W (Weyl gate) s.t.
U(0,1) = c2r(0) c2l(1) W(0,1) c1r(0) c1l(1)this yields 4 1-qubit unitary gates.
W(0,1) = RXX(0,1) RYY(0,1) RZZ(0,1)RYY(0,1) = sdg(0) sdg(0,1) RXX(0,1) s(0,1) s(0,1)RZZ(0,1) = h(0) h(0,1) h(0,1) h(0,1) h(0,1)yielding 8 additional 1-qubit unitary gates.
RXX(0,1) = k2r(0) k2l(1) Equiv(0,1) k1r(0) k1l(1)yielding 3*4 1-qubit unitary gates.
This gives a total of:
3*4 + 8 + 4 = 24 1-qubit unitary gates.
In this PR we multiply the 1-qubit unitary matrices betwen the 2-qubit gates, to get at most 8 1-qubit unitary gates (in the general case where 3 2-qubit gates are needed).
AI/LLM disclosure