Update single-qubit cliffords to fix XZ decomposition. #3414
Conversation
Also adds a test to ensure we are building the full group with either decomposition, and use ZXZ to ensure that the XZ decomposition uses only a single microwave gate per clifford.
| assert rms_err < 0.1 | ||
|
|
||
|
|
||
| def test_single_qubit_cliffords(): |
There was a problem hiding this comment.
Optional: It seems easy to write a unit test that verifies the key property of the Clifford group - that it fixes the Pauli group - but it seems we don't have it. WDYT about adding it?
(Strictly speaking this isn't related to this PR, but if we had this test then observing it pass before and after would provide a lot of confidence in the new logic.)
There was a problem hiding this comment.
I agree we should have some better abstractions around the pauli group and clifford group, but I think I'll leave that as future work because we'll probably need some design discussion around that.
| [Y**0.5], | ||
| [X**-0.5, Y**-0.5, X**0.5], | ||
| [Y, X**0.5], | ||
| ] # type: List[List[ops.Gate]] |
There was a problem hiding this comment.
nit: Maybe add comments up where Cliffords named tuple type is defined explaining the meaning of the five fields.
There was a problem hiding this comment.
Changed Cliffords to a dataclass and added some documentation about what these are.
| assert rms_err < 0.1 | ||
|
|
||
|
|
||
| def test_single_qubit_cliffords(): |
There was a problem hiding this comment.
Since one of your goals was to ensure at most one microwave gate in each decomposition, could you add a test for this? Otherwise, this can change on you later unexpectedly.
maffoo
added
the
automerge
|
Automerge cancelled: No approved review. |
CirqBot
removed
the
automerge
viathor
left a comment
viathor
left a comment
There was a problem hiding this comment.
Test is duplicated, see comment. Otherwise, LGTM.
| assert num_x <= 1 | ||
|
|
||
|
|
||
| def test_single_qubit_cliffords_xz_at_most_one_x_gate_per_clifford(): |
There was a problem hiding this comment.
The contents of this test duplicate the one above. I suppose you meant to move the for loop on line 76 above into here (and remove what we have in this test now)?
There was a problem hiding this comment.
Whoops! Cleaned this up.
viathor
left a comment
viathor
left a comment
There was a problem hiding this comment.
The new Cliffords type is really nice! Thank you!
3 participants
Also adds a test to ensure we are building the full group with either
decomposition, and use ZXZ to ensure that the XZ decomposition uses only
a single microwave gate per clifford.
Fixes #3411