Resolve inconsistencies in using controlled gates & controlled operations #4167

tanujkhattar · 2021-06-07T16:59:20Z

This PR tries to resolve the inconsistencies mentioned in #4172. Specifically,

Equality b/w controlled TOFFOLIs with different qubit ordering on controls, i.e. TOFFOLI(a,b,c).controlled_by(d) == TOFFOLI(d,b,c).controlled_by(a) (and other 3Q controlled gates like CCZ, CSWAP). To achieve this, we override the controlled method on CCX, CCZ etc. to return a ControlledGate with sub_gate = X in case of CCX s.t. TOFFOLI(a,b,c).controlled_by(d) == CCCX(a, b, d, c) == TOFFOLI(d,b,c).controlled_by(a) instead of CTOFFOLI
gate_operation.controlled_by now forwards the request to gate.controlled to first create a controlled gate and then apply it on qubits to create a ControlledOperation. This solves the original use case of adding specialized controls, requested in On PartialGate and cirq.Z(a).controlled_by(b) returning a CZ #2142, i.e. cirq.Z(q0).controlled_by(q1) == cirq.CZ(q1, q0).
Fixes ControlledOperation diagram draws exponent on the wrong qubits #4515

Note, this is a breaking change because

gate_operation.controlled_by() can now return a cirq.GateOperation instead of cirq.ControlledOperation in cases where the underlying gates have specialized gate.controlled() implementations.
- This also leads to a change in diagram of the controlled gates with specialized controlled implementations. For eg: Controlled S gate is now plotted as CZPowGate (@ --- @ ** 0.5) instead of ControlledOperation with Z ** 0.5 as subgate(@ ---- S)
op.controlled_by for 3Q gates like CCX, CCZ, CSWAP will now return ControlledOperation with sub_operation = <underlying non-controlled gate>. Eg: CCCX (i.e. sub_gate = X) instead of CTOFFOLI (i.e. sub_gate = TOFFOLI) etc.
Diagrams for ControlledOperations will now always have the exponent drawn on the target qubit (in case of multi qubit sub_operation, the exponent will always be on the first qubit if not the underlying gate does not explicitly specify a target index).

balopat

Two comments for discussion before we move forward:

This is technically a breaking change as user code might depend on the type of the controlled_by method being a ControlledGate. So we should make it very explicit in our release notes and label this PR as a breaking change.
Is there really no value at all in subsequent layers of ControlledGates? It feels like a very generic structure that maybe some decomposition code could leverage. But with jumping directly to "specialized" implementation we lose this structure. Maybe could have both? I.e. from a (multi-)controlled gate we could have a method reduce_type, simplify_type or specialize_type that could return the equivalent type in the hierarchy as needed based on the underlying gate if there is one...I.e. we would do the delegation to the subgate only on demand - this pattern would be similar to simplify Sympy expressions.

tanujkhattar · 2021-06-08T01:39:23Z

Thanks for the feedback. I found some more cases and have opened #4172 to discuss the various issues related to controlled gates & operations.
I'll mark this PR as draft for now till the discussion on the issue is resolved.

tanujkhattar · 2021-07-09T14:09:59Z

@balopat This is ready for review. I'm open to suggestions for a better breaking change description in the PR.

balopat

I feel like that the consistency tests should follow the pattern in cirq.testing.assert_implements_consistent_protocols -- i.e. I'd create an assertion in cirq.testing that takes a gate, and as sequence of pows, and then do the two consistency tests on the gate and the gate powers. Then I'd use this assert method for all of our gates to achieve full coverage across all our gates.

balopat · 2021-07-09T15:30:00Z

Re: breaking change note - looks good, but I would specifically mention in the breaking changes the consequent change in QFT and quirk diagrams controlled operations. Some people depend on diagrams for their tests...

tanujkhattar · 2021-07-12T19:51:10Z

@balopat I've modified the PR description and added assert_controlled_and_controlled_by_identical as suggested. PTAL.

Also, do you want me to add 100% coverage across all Cirq gates for assert_controlled_and_controlled_by_identical in this PR itself? Or could it be a follow-up PR ?

tanujkhattar · 2021-08-11T18:14:59Z

@balopat Ping, this is ready for re-review. PTAL.

tanujkhattar · 2021-08-20T17:35:47Z

@MichaelBroughton Can you please take a look at this?

…uliString's

tanujkhattar · 2021-09-25T00:49:33Z

Also added a fix for #4515 in this PR to avoid breaking the diagrams twice. See #4529 (comment) for justification.

This PR is again ready for re-review

MichaelBroughton

I'm ready to merge on this. @maffoo or @smitsanghavi , any final thoughts ?

smitsanghavi

LGTM

…ers to support other operation types. (#4459) Proliferation of `isinstance(op, GateOperation)` checks results in many inconsistencies due to different available operation types like `ControlledOperations` and `TaggedOperations`. This PR fixes #4152 and is a first step towards fixing #3556 Note that `TaggedOperations` which were earlier ignored by the optimizers would now be considered, and hence this is potentially a breaking change if people were implicitly relying on TaggedOperations not getting compiled by the optimizers. Since the optimizer doesn't document / test this behavior, I consider it to be a bug rather than a feature and an explicit `NoCompile` tag should be implemented as part of #4253 This PR is blocked on submitting #4167 (tests will stop failing once the PR is submitted and this rebased). Update: This is now ready for review.

…lib#4462) `CX` and `CCX` gates currently print the exponent on the bottom most qubit in the circuit diagram. This PR modifies this behavior to always draw the exponent on the target qubit, which is also the expected behavior for other `ControlledOperations`. This came up in discussions on quantumlib#4167 (comment) Note that behavior of `CZ` and `CCZ` is left unchanged, i.e. draws exponent on the bottom most qubit, since these gates are symmetrical across all applied qubits. BREAKING CHANGE: Tests depending upon diagrams of `CX` and `CCX` can now fail.

…ions (quantumlib#4167) This PR tries to resolve the inconsistencies mentioned in quantumlib#4172. Specifically, - Equality b/w controlled `TOFFOLI`s with different qubit ordering on controls, i.e. `TOFFOLI(a,b,c).controlled_by(d) == TOFFOLI(d,b,c).controlled_by(a)` (and other 3Q controlled gates like CCZ, CSWAP). To achieve this, we override the `controlled` method on `CCX`, `CCZ` etc. to return a `ControlledGate` with `sub_gate = X` in case of `CCX` s.t. `TOFFOLI(a,b,c).controlled_by(d) == CCCX(a, b, d, c) == TOFFOLI(d,b,c).controlled_by(a)` instead of `CTOFFOLI` - `gate_operation.controlled_by` now forwards the request to `gate.controlled` to first create a controlled gate and then apply it on qubits to create a `ControlledOperation`. This solves the original use case of adding specialized controls, requested in quantumlib#2142, i.e. `cirq.Z(q0).controlled_by(q1) == cirq.CZ(q1, q0)`. - Fixes quantumlib#4515 Note, this is a breaking change because - `gate_operation.controlled_by()` can now return a `cirq.GateOperation` instead of `cirq.ControlledOperation` in cases where the underlying gates have specialized `gate.controlled()` implementations. - This also leads to a change in diagram of the controlled gates with specialized controlled implementations. For eg: Controlled S gate is now plotted as `CZPowGate` (`@ --- @ ** 0.5`) instead of `ControlledOperation` with Z ** 0.5 as subgate(`@ ---- S`) - `op.controlled_by` for 3Q gates like `CCX`, `CCZ`, `CSWAP` will now return `ControlledOperation` with `sub_operation = <underlying non-controlled gate>`. Eg: `CCCX` (i.e. `sub_gate = X`) instead of `CTOFFOLI` (i.e. `sub_gate = TOFFOLI`) etc. - Diagrams for `ControlledOperations` will now always have the exponent drawn on the target qubit (in case of multi qubit `sub_operation`, the exponent will always be on the first qubit if not the underlying gate does not explicitly specify a target index).

…ers to support other operation types. (quantumlib#4459) Proliferation of `isinstance(op, GateOperation)` checks results in many inconsistencies due to different available operation types like `ControlledOperations` and `TaggedOperations`. This PR fixes quantumlib#4152 and is a first step towards fixing quantumlib#3556 Note that `TaggedOperations` which were earlier ignored by the optimizers would now be considered, and hence this is potentially a breaking change if people were implicitly relying on TaggedOperations not getting compiled by the optimizers. Since the optimizer doesn't document / test this behavior, I consider it to be a bug rather than a feature and an explicit `NoCompile` tag should be implemented as part of quantumlib#4253 This PR is blocked on submitting quantumlib#4167 (tests will stop failing once the PR is submitted and this rebased). Update: This is now ready for review.

…lib#4462) `CX` and `CCX` gates currently print the exponent on the bottom most qubit in the circuit diagram. This PR modifies this behavior to always draw the exponent on the target qubit, which is also the expected behavior for other `ControlledOperations`. This came up in discussions on quantumlib#4167 (comment) Note that behavior of `CZ` and `CCZ` is left unchanged, i.e. draws exponent on the bottom most qubit, since these gates are symmetrical across all applied qubits. BREAKING CHANGE: Tests depending upon diagrams of `CX` and `CCX` can now fail.

…ions (quantumlib#4167) This PR tries to resolve the inconsistencies mentioned in quantumlib#4172. Specifically, - Equality b/w controlled `TOFFOLI`s with different qubit ordering on controls, i.e. `TOFFOLI(a,b,c).controlled_by(d) == TOFFOLI(d,b,c).controlled_by(a)` (and other 3Q controlled gates like CCZ, CSWAP). To achieve this, we override the `controlled` method on `CCX`, `CCZ` etc. to return a `ControlledGate` with `sub_gate = X` in case of `CCX` s.t. `TOFFOLI(a,b,c).controlled_by(d) == CCCX(a, b, d, c) == TOFFOLI(d,b,c).controlled_by(a)` instead of `CTOFFOLI` - `gate_operation.controlled_by` now forwards the request to `gate.controlled` to first create a controlled gate and then apply it on qubits to create a `ControlledOperation`. This solves the original use case of adding specialized controls, requested in quantumlib#2142, i.e. `cirq.Z(q0).controlled_by(q1) == cirq.CZ(q1, q0)`. - Fixes quantumlib#4515 Note, this is a breaking change because - `gate_operation.controlled_by()` can now return a `cirq.GateOperation` instead of `cirq.ControlledOperation` in cases where the underlying gates have specialized `gate.controlled()` implementations. - This also leads to a change in diagram of the controlled gates with specialized controlled implementations. For eg: Controlled S gate is now plotted as `CZPowGate` (`@ --- @ ** 0.5`) instead of `ControlledOperation` with Z ** 0.5 as subgate(`@ ---- S`) - `op.controlled_by` for 3Q gates like `CCX`, `CCZ`, `CSWAP` will now return `ControlledOperation` with `sub_operation = <underlying non-controlled gate>`. Eg: `CCCX` (i.e. `sub_gate = X`) instead of `CTOFFOLI` (i.e. `sub_gate = TOFFOLI`) etc. - Diagrams for `ControlledOperations` will now always have the exponent drawn on the target qubit (in case of multi qubit `sub_operation`, the exponent will always be on the first qubit if not the underlying gate does not explicitly specify a target index).

tanujkhattar requested review from cduck, vtomole and a team as code owners June 7, 2021 16:59

tanujkhattar requested a review from dstrain115 June 7, 2021 16:59

google-cla bot added the cla: yes Makes googlebot stop complaining. label Jun 7, 2021

balopat reviewed Jun 7, 2021

View reviewed changes

tanujkhattar changed the title ~~controlled_operation.gate() should return sub_operation.gate.controlled()~~ [DRAFTcontrolled_operation.gate() should return sub_operation.gate.controlled() Jun 8, 2021

tanujkhattar changed the title ~~[DRAFTcontrolled_operation.gate() should return sub_operation.gate.controlled()~~ [DRAFT] Resolve inconsistencies with controlled gates & controlled operations Jun 8, 2021

tanujkhattar changed the title ~~[DRAFT] Resolve inconsistencies with controlled gates & controlled operations~~ [Draft] Resolve inconsistencies with controlled gates & controlled operations Jun 8, 2021

tanujkhattar marked this pull request as draft June 8, 2021 01:40

balopat self-assigned this Jul 1, 2021

balopat removed the request for review from dstrain115 July 1, 2021 16:04

tanujkhattar force-pushed the controlled_op_gate_fix branch from 053138f to 11ece2c Compare July 9, 2021 13:44

tanujkhattar changed the title ~~[Draft] Resolve inconsistencies with controlled gates & controlled operations~~ Resolve inconsistencies in using controlled gates & controlled operations Jul 9, 2021

tanujkhattar marked this pull request as ready for review July 9, 2021 14:09

tanujkhattar requested a review from balopat July 9, 2021 14:09

tanujkhattar added the BREAKING CHANGE For pull requests that are important to mention in release notes. label Jul 9, 2021

balopat suggested changes Jul 9, 2021

View reviewed changes

cduck mentioned this pull request Jul 9, 2021

Inconsistencies in using ControlledGate(s) and ControlledOperation(s) #4172

Closed

tanujkhattar requested a review from balopat July 12, 2021 19:51

CirqBot added size: XL lines changed >1000 size: L 250< lines changed <1000 and removed size: XL lines changed >1000 labels Aug 12, 2021

tanujkhattar assigned MichaelBroughton Aug 14, 2021

tanujkhattar added 6 commits September 24, 2021 17:18

Add controlled gate consistency test to assert consistent protocols suit

ed28674

Support qudits and consistency check and add special case for DensePa…

56130b3

…uliString's

Fix lint

bc8f2d4

Draw exponent on target in tests

9bc1861

Fix controlled gate tests which depend upon controlled op

d947b66

Fix lint

7c83f33

tanujkhattar force-pushed the controlled_op_gate_fix branch from c340eac to 7c83f33 Compare September 25, 2021 00:45

tanujkhattar mentioned this pull request Sep 25, 2021

Fix diagrams for ControlledOperations and QuirkInputRotationOperation #4529

Closed

Add type annotations and parameterize test

a6f8cb0

MichaelBroughton approved these changes Sep 30, 2021

View reviewed changes

smitsanghavi approved these changes Oct 1, 2021

View reviewed changes

MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Oct 1, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Oct 1, 2021

Merge branch 'master' into controlled_op_gate_fix

87dd128

CirqBot merged commit f48efe0 into quantumlib:master Oct 1, 2021

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Oct 1, 2021

tanujkhattar deleted the controlled_op_gate_fix branch October 1, 2021 18:56

MichaelBroughton mentioned this pull request Oct 17, 2021

Increment requirement to Cirq to 0.13.1 tensorflow/quantum#621

Merged

mpharrigan mentioned this pull request Oct 21, 2021

Update to Cirq 0.13 quantumlib/ReCirq#224

Closed

5 tasks

tonybruguier mentioned this pull request Oct 21, 2021

Handle 3-qubit gates tensorflow/quantum#636

Open

dstrain115 mentioned this pull request Oct 25, 2021

Toffoli is no longer a controlled operation in cirq 0.13 quantumlib/ReCirq#225

Merged

tanujkhattar mentioned this pull request Aug 9, 2023

Controlled CCX / CCZ gates do not respect global shift #6235

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Resolve inconsistencies in using controlled gates & controlled operations #4167

Resolve inconsistencies in using controlled gates & controlled operations #4167

Uh oh!

tanujkhattar commented Jun 7, 2021 •

edited

Loading

Uh oh!

balopat left a comment

Uh oh!

tanujkhattar commented Jun 8, 2021

Uh oh!

tanujkhattar commented Jul 9, 2021

Uh oh!

balopat left a comment •

edited

Loading

Uh oh!

balopat commented Jul 9, 2021

Uh oh!

tanujkhattar commented Jul 12, 2021

Uh oh!

tanujkhattar commented Aug 11, 2021

Uh oh!

tanujkhattar commented Aug 20, 2021

Uh oh!

tanujkhattar commented Sep 25, 2021

Uh oh!

MichaelBroughton left a comment

Uh oh!

smitsanghavi left a comment

Uh oh!

Uh oh!

Resolve inconsistencies in using controlled gates & controlled operations #4167

Resolve inconsistencies in using controlled gates & controlled operations #4167

Uh oh!

Conversation

tanujkhattar commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

tanujkhattar commented Jun 8, 2021

Uh oh!

tanujkhattar commented Jul 9, 2021

Uh oh!

balopat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balopat commented Jul 9, 2021

Uh oh!

tanujkhattar commented Jul 12, 2021

Uh oh!

tanujkhattar commented Aug 11, 2021

Uh oh!

tanujkhattar commented Aug 20, 2021

Uh oh!

tanujkhattar commented Sep 25, 2021

Uh oh!

MichaelBroughton left a comment

Choose a reason for hiding this comment

Uh oh!

smitsanghavi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tanujkhattar commented Jun 7, 2021 •

edited

Loading

balopat left a comment •

edited

Loading