Skip to content
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

Speed up execution time of merge_single_qubit_moments_to_phxz transformer by avoiding redundant calls to unitary protocol #6174

21 changes: 17 additions & 4 deletions cirq-core/cirq/transformers/merge_single_qubit_gates.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,23 @@ def merge_func(m1: 'cirq.Moment', m2: 'cirq.Moment') -> Optional['cirq.Moment']:
return None
ret_ops = []
for q in m1.qubits | m2.qubits:
mat = protocols.unitary(circuits.Circuit(m.operation_at(q) or [] for m in [m1, m2]))
gate = single_qubit_decompositions.single_qubit_matrix_to_phxz(mat, atol)
if gate:
ret_ops.append(gate(q))
op1, op2 = m1.operation_at(q), m2.operation_at(q)
if op1 and op2:
mat = protocols.unitary(op2) @ protocols.unitary(op1)
gate = single_qubit_decompositions.single_qubit_matrix_to_phxz(mat, atol)
if gate:
ret_ops.append(gate(q))
Comment on lines +134 to +135
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this can return an empty moment if this condition and another at line 145 are False.

Does the merge_moments function below handle such case and keeps the original moments unchanged?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems merge_moments expects merge_func returns None when moments cannot be merged, please correct the return value below.

Ref:

if merged_moment is None:
merged_moments.append(current_moment)
else:
merged_moments[-1] = merged_moment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single_qubit_matrix_to_phxz returns None when the matrix corresponds to an identity gate (it doesn't represent a failure to construct a PhasedXZGate).

If all merged operations correspond to identity, it is perfectly reasonable to return an empty moment corresponding to the "merged" moment from the merge_func and thus replace two moments containing single qubit gates with a single empty moment in the circuit.

Note that we already check whether the two moments m1 and m2 are merge-able or not in the can_merge_moment condition at the beginning of merge_func. Thus, no additional changes are needed here and the behavior is as intended.

else:
op = op1 or op2
assert op is not None
if isinstance(op.gate, ops.PhasedXZGate):
ret_ops.append(op)
else:
gate = single_qubit_decompositions.single_qubit_matrix_to_phxz(
protocols.unitary(op), atol
)
if gate:
ret_ops.append(gate(q))
return circuits.Moment(ret_ops)

return transformer_primitives.merge_moments(
Expand Down