-
Notifications
You must be signed in to change notification settings - Fork 984
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
Convert single-qubit gates to single-gate PhasedXZ for sycamore #2796
Conversation
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 pending some minor comments.
mat: np.ndarray, | ||
atol: float = 0, | ||
) -> Optional[ops.SingleQubitGate]: | ||
"""Implements a single-qubit operation with a PhasedXZ gate. |
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.
Should we add information about the way this is computed?
cirq/optimizers/decompositions.py
Outdated
return None | ||
|
||
# Special case: XY half-turns can absorb Z rotations. | ||
if abs(xy_turn) >= 0.5 - atol: |
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.
Is this correct? Do you mean xy_turn more than 0.5 or xy_turn within atol of 0.5?
(or does xy_turn max out at 0.5?)
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.
xy_turn
is in the range [-0.5, 0.5]
thanks to the _signed_mod_1
function, so this is equivalent to checking if abs(xy_turn)
is close to 0.5 (it's also done this way in the existing function single_qubit_matrix_to_phased_x_z
above. But I agree this is a bit confusing; will change to use math.isclose
.
) -> None: | ||
"""Canonicalizes runs of single-qubit rotations in a circuit. | ||
|
||
Specifically, any run of non-parameterized circuits will be replaced by an |
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.
Would "any run of non-parameterized single-qubit operations" be more precise?
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.
That's definitely better; fixed.
gate = decompositions.single_qubit_matrix_to_phxz(matrix, atol) | ||
return [gate(qubit)] if gate else [] | ||
|
||
MergeSingleQubitGates(synthesizer=synth).optimize_circuit(circuit) |
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.
Is this tested anywhere? If so, I missed 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.
It was being tested indirectly since it gets used in optimized_for_sycamore
; will add an explicit test for just this optimizer.
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
I've added a separate decomposition and merge optimizer for the
PhasedXZ
case. I think ideally this would just replace the existing code which emits separatePhasedXPow
andZ
gates, but that is currently used in, for example, the ion trap decompositions where it is not clear that these devices and/or APIs supportPhasedXZ
. Once this is in I have a separate PR to add support forPhasedXZ
in theEjectZ
optimizer.