Skip to content

Attempt canonicalization first when decomposing controlled gates #7242 #7269

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Attempt canonicalization first when decomposing controlled gates #7242 #7269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
551b87d
Attempt canonicalization first when decomposing controlled gates.
codrut3 Apr 13, 2025
416bafd
Be precise which 2-cycle needs to be avoided in the decomposition.
codrut3 Apr 13, 2025
8e38581
Add a comment that the if condition can be removed once #7241 is reso…
codrut3 Apr 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cirq-core/cirq/ops/controlled_gate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,17 @@ 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()])
controlled_sub_gate = self.sub_gate.controlled(
self.num_controls(), self.control_values, self.control_qid_shape
)
# Prefer the subgate controlled version if available
if self != controlled_sub_gate:
# Prevent 2-cycle from appearing in the recursive decomposition
# TODO: Remove after #7241 is resolved
if not isinstance(controlled_sub_gate, ControlledGate) or not isinstance(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this condition possible? The constructor for ControlledGate absorbs the control layers if the subgate is another ControlledGate. Do any of the unit tests fail if this check is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried first without and the unit tests were failing.

I discovered that a 2-cycle forms in the decomposition for something like CCZ and control_qid_shape = [3].
The following happens:

  • when the sub gate is CZ, CZPowGate.controlled() sees that control_qid_shape[-1] != 2 and returns a ControlledGate. Then self != controlled_sub_gate is false. Next, the special case in ControlledGate._decompose_with_context_() for CZPowGate creates a Z gate and adds +1 to the control qubits;
  • when the sub gate is Z, the ZPowGate.controlled() method returns a CZPowGate and subtracts -1 from the control qubits. Then self != controlled_sub_gate is true.

This cycles infinitely, adding and removing a control qubit.

controlled_sub_gate.sub_gate, common_gates.CZPowGate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do any of the unit tests depend on this condition? If #7241 is done, does that mitigate the need for this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, can you check the example in the comment above and see if it would work when #7241 is resolved?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will give it a look tonight if I get a chance

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, seems like if we make the end of CXPowGate.controlled look something like

        if self._global_shift != 0 or not isinstance(result, controlled_gate.ControlledGate):
            return result
        if (
            isinstance(result.control_values, cv.ProductOfSums)
            and result.control_values[-1] == (1,)
            and result.control_qid_shape[-1] == 2
        ):
            return cirq.CCXPowGate(
                exponent=self._exponent, global_shift=self._global_shift
            ).controlled(
                result.num_controls() - 1, result.control_values[:-1], result.control_qid_shape[:-1]
            )
        return controlled_gate.ControlledGate(
            XPowGate(exponent=self.exponent),
            num_controls=result.num_controls() + 1,
            control_values=result.control_values & cv.ProductOfSums([1]),
            control_qid_shape=result.control_qid_shape + (2,),
        )

(basically updating the final return value to be a controlled X with an extra control value, instead of a controlled CX), and similar for CZPowGate, then that mitigates the need for either of those checks.

I think any of the following approaches would work:

  1. Do Eliminate multiple control layers on CX/CZ.controlled([0]) #7241 first and pull that into here once it's been merged.
  2. Do Eliminate multiple control layers on CX/CZ.controlled([0]) #7241 and Attempt canonicalization first when decomposing controlled gates #7242 both here in the same PR.
  3. Just add a comment to your if conditions that they can be removed once Eliminate multiple control layers on CX/CZ.controlled([0]) #7241 is complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to add a comment and then remove the check when doing #7241, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That works. An ulterior motive for the set of issues I posted recently is to remove all the casting and type checking from ControlledGate.decompose. Once all four are complete, I'm pretty sure all of the type checks can be eliminated from that function, and things will generally be more consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I added a comment.

):
return controlled_sub_gate.on(*qubits)
if (
protocols.has_unitary(self.sub_gate)
and protocols.num_qubits(self.sub_gate) == 1
Expand Down
30 changes: 30 additions & 0 deletions cirq-core/cirq/ops/controlled_gate_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,36 @@ def _test_controlled_gate_is_consistent(
np.testing.assert_allclose(cirq.unitary(cgate), cirq.unitary(circuit), atol=1e-13)


@pytest.mark.parametrize(
'sub_gate, expected_decomposition',
[
(cirq.X, [cirq.CX]),
(cirq.CX, [cirq.CCX]),
(cirq.XPowGate(), [cirq.CXPowGate()]),
(cirq.CXPowGate(), [cirq.CCXPowGate()]),
(cirq.Z, [cirq.CZ]),
(cirq.CZ, [cirq.CCZ]),
(cirq.ZPowGate(), [cirq.CZPowGate()]),
(cirq.CZPowGate(), [cirq.CCZPowGate()]),
],
)
def test_controlled_gate_decomposition_uses_canonical_version(sub_gate, expected_decomposition):
cgate = cirq.ControlledGate(sub_gate, num_controls=1)
qubits = cirq.LineQubit.range(1 + sub_gate.num_qubits())
dec = cirq.decompose_once(cgate.on(*qubits))
assert [op.gate for op in dec] == expected_decomposition


@pytest.mark.parametrize(
'sub_gate, expected_decomposition', [(cirq.Z, [cirq.CZ]), (cirq.ZPowGate(), [cirq.CZPowGate()])]
)
def test_controlled_gate_full_decomposition(sub_gate, expected_decomposition):
cgate = cirq.ControlledGate(sub_gate, num_controls=1)
qubits = cirq.LineQubit.range(1 + sub_gate.num_qubits())
dec = cirq.decompose(cgate.on(*qubits))
assert [op.gate for op in dec] == expected_decomposition


def test_pow_inverse():
assert cirq.inverse(CRestricted, None) is None
assert cirq.pow(CRestricted, 1.5, None) is None
Expand Down