Change merge_single_qubit_moments_to_phxz to support global phase.

[codrut3]

This addresses an issue I found while working on #7291

Currently merge_single_qubit_moments_to_phxz will not merge moments that have a global phase, meaning that moments that could theoretically be merged are not. I updated it so that global phase is also supported. I added two tests to show the issue.

Perhaps this situation is not common right now, but if #7383 is committed and the flag is used, it may become more common. @daxfohl

Note that I'm not merging the global phase operations in a single one, because I'm thinking some phases could be parametrized; should I attempt to do this?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.68%. Comparing base (e27d883) to head (47a442f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7405   +/-   ##
=======================================
  Coverage   98.68%   98.68%           
=======================================
  Files        1112     1112           
  Lines       97709    97754   +45     
=======================================
+ Hits        96427    96472   +45     
  Misses       1282     1282           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[daxfohl]

Note that I'm not merging the global phase operations in a single one, because I'm thinking some phases could be parametrized; should I attempt to do this?

In theory, yes, but as things currently are, probably not. It's fine to merge parameterized gates; a coefficient of e.g. a + b + 0.3 will still work fine in sweeps if values for a and b are provided. But the problem is that global phase isn't the only kind of zero-qubit gate. There's MatrixGate, DiagonalGate, various varadic-qubit-count gates like BooleanHamiltonian, Identity, PauliStringPhasor, etc. In theory they could all be converted to global phase by getting the 1x1 unitary, but parameterized gates generally return NotImplemented for the _unitary_ implementation. Plausibly there could be timing gates or pure classical logic gates or debug "gates" that don't even have unitaries. So, while ideally we could merge them, I don't think things are in a state currently where we could do so. Plausibly "have all zero-qubit gates 'decompose' to a global phase gate" could be a new feature request, which would make this more feasible.

Note that in #7383 the default behavior is unchanged; the phase shift doesn't get extracted during decomposition except when it's controlled, or if users explicitly set the phase extraction flag.

I'd say in this one, it maybe should have a flag to control this behavior as well? If users are explicitly setting the phase extraction flag in decomposition, then maybe they want to keep the phases in separate moments. Or if they have custom gates like pure classical logic or timing, they might not want those to collapse to a single moment by default. Hard to say for sure. Maybe get some feedback from @tanujkhattar who did the initial implementation?

[daxfohl]

<= 1?

[codrut3]

Done.

[codrut3]

I'd say in this one, it maybe should have a flag to control this behavior as well? If users are explicitly setting the phase extraction flag in decomposition, then maybe they want to keep the phases in separate moments. Or if they have custom gates like pure classical logic or timing, they might not want those to collapse to a single moment by default. Hard to say for sure. Maybe get some feedback from @tanujkhattar who did the initial implementation?

Thank you a lot for your reply @daxfohl ! Do you suggest that merging global phases should be behind a flag?

I think the benefit of this PR is that it allows merging more moments than before. merge_single_qubit_moments_to_phxz is called during conversion to a target gateset, so I believe this would improve some circuits. The only example I had came after extracting global phase in #7291, but it's possible there are circuits right now that would benefit from this PR.

[daxfohl]

Do you suggest that merging global phases should be behind a flag?

No, I wouldn't worry about merging the global phases. That could be a subsequent change or even a separate transformer. What I means is that I think the functionality that's already in this PR should maybe require a parameter to enable it. I'm not terribly worried about it causing problems with global phase ops getting moved around, or that the extra efficiency allowing more ops to be placed in a moment would cause problems. Those aspects should be fine. I'm more concerned that things like classical logic gates, timing gates, or other random zero-qubit non-global-phase "utility" gates would also be affected, possibly in a breaking way.

That concern comes with a caveat though: IDK whether google's internal repos define or use such types of zero-qubit utility gates (I can't think of any such gates in cirq proper, but I know qiskit and q# both have things like this). If not, then probably this feature can go as-is. But if so, then the effect of this change needs to be considered for such gates.

[codrut3]

No, I wouldn't worry about merging the global phases. That could be a subsequent change or even a separate transformer. What I means is that I think the functionality that's already in this PR should maybe require a parameter to enable it. I'm not terribly worried about it causing problems with global phase ops getting moved around, or that the extra efficiency allowing more ops to be placed in a moment would cause problems. Those aspects should be fine. I'm more concerned that things like classical logic gates, timing gates, or other random zero-qubit non-global-phase "utility" gates would also be affected, possibly in a breaking way.

Ah, I understand. There is a risk there is a gate that is moment-dependent (or position-dependent). Can you link an example from Qiskit? I'll have a look if something like this exists in Cirq.

[daxfohl]

Something like https://docs.quantum.ibm.com/api/qiskit/0.24/qiskit.circuit.Delay for instance.

[codrut3]

There is WaitGate, but it is on >=1 qubit.

I can change the merge condition to return true only if the zero qubit gates in the moment are global phase gates. What do you think?

[daxfohl]

I'd say probably add a protocols.has_unitary check on it. That'd give a good indication that it's either a global phase gate or something that behaves like one (1x1 MatrixGate, single-cell DiagonalGate, empty IdentityGate or PauliString, etc), and avoids explicit type checking. While it's plausible that someone could.create a utility gate with some unitary that goes along with it, I'd think it's rather unlikely, so I'd say the has_unitary check is sufficient.

[codrut3]

Thank you! I see this is already part of the condition on line 122:

and protocols.has_unitary(op)

One other possible issue is that the documentation talks only about 1-qubit rotations. Would the user expect zero-qubit gates to be merged too? This was my initial reaction (why isn't global phase considered?), but maybe the user expectation is to ignore moments that have anything else in them.

[daxfohl]

I see this is already part of the condition on line 122

Doh! <head smack> Can't believe I missed that. Okay yeah that greatly alleviates that concern. It would also ostensibly mean they could all be merged since they should all be 1x1 unitaries, which can just be multiplied to get a single global phase coefficient. Though still, I don't necessarily see a need to do so; it could be a separate PR or even a separate transformer.

Would the user expect zero-qubit gates to be merged too?

Yeah, this is where it goes out of my domain of knowledge. I'd have no basis even to make a guess. Probably need to talk to @tanujkhattar or someone who works with this transformer.