Simplify decomposition of controlled eigengates with global phase #7291
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7291 +/- ##
==========================================
+ Coverage 98.64% 98.66% +0.01%
==========================================
Files 1106 1106
Lines 95985 96144 +159
==========================================
+ Hits 94688 94864 +176
+ Misses 1297 1280 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -159,6 +160,12 @@ def _decompose_with_context_( | |||
| self, qubits: Tuple['cirq.Qid', ...], context: Optional['cirq.DecompositionContext'] = None | |||
| ) -> Union[None, NotImplementedType, 'cirq.OP_TREE']: | |||
| control_qubits = list(qubits[: self.num_controls()]) | |||
| # If the subgate is an EigenGate with non-zero phase, try to decompose it | |||
| # into a phase-free gate and a global phase gate. | |||
| if isinstance(self.sub_gate, eigen_gate.EigenGate) and self.sub_gate.global_shift != 0: | |||
There was a problem hiding this comment.
Is there any way you can get rid of this condition? Like try decomposing the subgate first, then only enter the branch if the subgate has decomposed? A big goal is to reduce the amount of type checking needed here.
If this works, then the duplicate function call at the end of this function can be removed, and the function can just return NotImplemented if it gets to that point.
| # their own _decompose_ method. | ||
| self_without_phase._global_shift = 0 | ||
| global_phase = 1j ** (2 * self.global_shift * self.exponent) | ||
| return [self_without_phase.on(*qubits), global_phase_op.GlobalPhaseGate(global_phase)()] |
There was a problem hiding this comment.
Maybe check that the remaining phase isn't zero, and drop it if it is. This could be the case if shift==0.5 and exponent==4 for instance. The decomposition would factor out the shift, leaving, say, X**4 and an identity phase gate that there's no reason to keep.
| @@ -473,7 +473,8 @@ def test_decompose(): | |||
| op = cirq.H(q0).with_classical_controls('a') | |||
| assert cirq.decompose(op) == [ | |||
| (cirq.Y(q0) ** 0.5).with_classical_controls('a'), | |||
| cirq.XPowGate(exponent=1.0, global_shift=-0.25).on(q0).with_classical_controls('a'), | |||
| cirq.X(q0).with_classical_controls('a'), | |||
There was a problem hiding this comment.
Yeah, so here's where we probably need to have an opt in flag on the decompose context object. Having the basic gates suddenly decompose to multiple gates by default probably would be too likely to break something. While generally it seems that changing details around complex decompositions have been approved, this change seems a little too fundamental.
Fixes #7238 following the recommendations in the issue description: I added a
_decompose_method toEigenGatethat extracts global phase, and updatedControlledGate._decompose_with_context_to try to extract the global phase first.This PR may be undesirable because gates that fix
global_shiftneed to implement their own_decompose_method. I did this forRx,RyandRz, but I don't have a nice solution for a not-yet implemented gate that derives fromEigenGateand fixesglobal_shift: any such gate must have a_decompose_method. See the discussion in #7238.While implementing this change I found two issues:
assert [*tagged_h._decompose_()] == cirq.decompose_once(h).tagged_h._decompose_callsdecompose_once, so it must be compared withcirq.decompose_once, not withcirq.decompose. The difference became visible when a second step in the decomposition extracted the global phase.merge_single_qubit_moments_to_phxzdoesn't handle well global phase. A global phase gate gets assigned to the first moment, and in the current implementation this makes it unmergeable with the next moment. I found this while examining a test failure. I fixed this, and created a test case to show the problem.It may make sense to address these issues in separate PRs, independent of this one. Let me know!