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

[tanujkhattar]

Step towards fixing #6173

Timing before my PR:

'circuit_rev': 0.31624889373779297
'inverse_circuit': 0.5580418109893799
'Merged circuit': 2.517353057861328
transformer for hardware inverse: 3.393893241882324
'cirq.inverse': 0.03910088539123535

Timing after my PR:

'circuit_rev': 0.31432485580444336
'inverse_circuit': 0.5529079437255859
'Merged circuit': 0.6783010959625244
transformer for hardware inverse: 1.5484230518341064
'cirq.inverse': 0.036971092224121094

[maffoo]

This will still be slower than it needs to be if there are more than two single-qubit moments in a row. Could we change it to instead collect runs of mergeable moments, then call the merge func with any number of moments: def merge_func(*moments: 'cirq.Moment') -> Optional['cirq.Moment']:?

[tanujkhattar]

That would involve changing the API of cirq.merge_moments primitive, which will be a backwards incompatible change. In future, we can consider adding a new primitive that expects a merge_func(*moments: 'cirq.Moment') instead of merge_func(cirq.Moment, cirq.Moment) but I'd like to punt it for a later PR.

Also, I chatted with @AlMrvn offline and have proposed a different implementation of InverseSycamoreCircuit that does not use these primitives and is much faster and should be enough to unblock him.

[maffoo]

That would involve changing the API of cirq.merge_moments primitive, which will be a backwards incompatible change. In future, we can consider adding a new primitive that expects a merge_func(*moments: 'cirq.Moment') instead of merge_func(cirq.Moment, cirq.Moment) but I'd like to punt it for a later PR.

Yes, I'd suggest a new function since we don't want to change the interface of merge_moments.

Also, I chatted with @AlMrvn offline and have proposed a different implementation of InverseSycamoreCircuit that does not use these primitives and is much faster and should be enough to unblock him.

Glad to hear it. This is what I suggested to him as well, that he'd probably be better off eschewing the standard transformers and writing something custom. In particular if it can avoid creating lots of intermediate circuits.

[pavoljuhas]

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?

[pavoljuhas]

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

Ref:

Cirq/cirq-core/cirq/transformers/transformer_primitives.py

Lines 543 to 546 in 7b753f6

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

[tanujkhattar]

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.

[pavoljuhas]

IIUC, merge_func needs to return None instead of an empty Moment.

[pavoljuhas]

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

Ref:

Cirq/cirq-core/cirq/transformers/transformer_primitives.py

Lines 543 to 546 in 7b753f6

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