Improve controlled gate decomposition logic and add targeted unit tests #7283
Conversation
| # is set equal to the phase angle. | ||
| # Handle global phase gates by checking if the unitary is a 1x1 matrix with a global phase | ||
| unitary = protocols.unitary(self.sub_gate, default=None) | ||
| if ( |
There was a problem hiding this comment.
I was thinking this branch could be merged with the above branch. The conditions protocols.has_unitary(self.sub_gate) and self._qid_shape_() == (2,) * len(self._qid_shape_()) apply to both. The global phase inner branch would then just need an extra check for protocols.num_qubits(self.sub_gate) == 0. The if protocols.is_parameterized(self.sub_gate) or set(shape) != {2}: check can be removed, because parameterized gates fail has_unitary, and set(shape) == {2} is equivalent to the self_qid_shape_() condition. There's also no need for the three conditions you added, since zero qubits implies a 1x1 unitary, and the abs of a unitary must square-sum to 1 by definition.
| # A controlled global phase is a diagonal gate, where each active control value index | ||
| # is set equal to the phase angle. | ||
| # Handle global phase gates by checking if the unitary is a 1x1 matrix with a global phase | ||
| unitary = protocols.unitary(self.sub_gate, default=None) |
There was a problem hiding this comment.
protocols.unitary can be expensive, and here it's being called for every subgate. Better to rely on protocols.has_unitary and protocols.num_qubits and/or protocols.qid_shape in the conditions, and then only call protocols.unitary from inside the branch.
There was a problem hiding this comment.
New PR should fix both this and the discussion above. I removed the extra unit test for now as I'm not sure if it is really necessary.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7283 +/- ##
==========================================
- Coverage 98.66% 98.66% -0.01%
==========================================
Files 1106 1106
Lines 96086 96098 +12
==========================================
+ Hits 94808 94819 +11
- Misses 1278 1279 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I think it still needs a unit test. Unlike the one you deleted, the unit test should directly check the new decomposition functionality. Previously, controlled GlobalPhaseGates would decompose but equivalent controlled MatrixGates wouldn't. So a unit test could check that controlled MatrixGate now decomposes to the same thing that an equivalent controlled GlobalPhaseGate does.
| and protocols.num_qubits(self.sub_gate) == 1 | ||
| and self._qid_shape_() == (2,) * len(self._qid_shape_()) | ||
| and isinstance(self.control_values, cv.ProductOfSums) | ||
| if protocols.has_unitary(self.sub_gate) and self._qid_shape_() == (2,) * len( |
There was a problem hiding this comment.
While you're here, can you change the second condition to all(q.dimension == 2 for q in qubits) since it's equivalent and more readable?
There was a problem hiding this comment.
Added the unit test + swapped order of branches since the matrix gate seems like more of an edge case. Doesn't change logic though.
Summary
Fixes #7248
This PR improves the decomposition logic for controlled gates in Cirq, ensuring that controlled 1x1
MatrixGateandGlobalPhaseGateinstances decompose correctly. It also adds a targeted unit tests to verify this behavior.Details
DiagonalGatewith the correct phase.Supersedes #7282