Skip to content

Conversation

daxfohl
Copy link
Collaborator

@daxfohl daxfohl commented Jun 15, 2021

Closes #2164

Copies Two/ThreeQubitGate to cirq.testing due to its convenience there, and deprecates the originals.

BREAKING CHANGE: @balopat @95-martin-orion @tanujkhattar During the deprecation period, isinstance(TwoQubitGate) has been shimmed such that it will now return True iff num_qubits() returns 2, regardless of whether it's actually part of the class hierarchy (and same for ThreeQubitGate). It's not expected that this will cause real-world breakages, but possible.

This can go before or after #4236, as they are disjoint problems.

@daxfohl daxfohl requested review from a team, cduck, vtomole and wcourtney as code owners June 15, 2021 16:32
@daxfohl daxfohl requested a review from viathor June 15, 2021 16:32
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Jun 15, 2021
Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

I think we should get rid of ThreeQubitGateTest and always define a class ThreeQubitGate when we need it. What do you think?

@daxfohl daxfohl marked this pull request as draft June 20, 2021 15:39
@daxfohl daxfohl changed the title Deprecate ThreeQubitGate [WIP] Deprecate ThreeQubitGate Jun 20, 2021
@daxfohl daxfohl changed the title [WIP] Deprecate ThreeQubitGate [WIP] Deprecate Two/ThreeQubitGate Jun 20, 2021
@daxfohl daxfohl marked this pull request as ready for review June 20, 2021 17:49
@daxfohl daxfohl changed the title [WIP] Deprecate Two/ThreeQubitGate Deprecate Two/ThreeQubitGate Jun 20, 2021
@daxfohl daxfohl requested review from tanujkhattar and removed request for balopat August 24, 2021 19:42
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

LGTM % nits.

@daxfohl
Copy link
Collaborator Author

daxfohl commented Aug 31, 2021

Wondering now if we should make Gate generic based on shape of supported qubits. So Gate[Tuple[Qubit, Qubit]] would be a two-qubit gate. That lets us be more explicitly typesafe without requiring multiple inheritance. There are some subtleties like ControlledGates, but maybe we could work it out.

@tanujkhattar
Copy link
Collaborator

I personally prefer the current structure of checking gate attributes (eg: via cirq.num_qubits) instead of checking gate types (eg: SingleQubitGate), and hence I don't see much value in making the gate generic based on qubits. Also, it'll be a very large breaking change, so we should have strong arguments to pursue it.

Let's merge this PR and discuss the proposal on a separate issue.

@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 31, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 31, 2021
@CirqBot CirqBot merged commit 7951f84 into quantumlib:master Aug 31, 2021
@CirqBot 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 Aug 31, 2021
@daxfohl daxfohl deleted the gates branch September 1, 2021 13:46
@vtomole
Copy link
Collaborator

vtomole commented Sep 1, 2021

Hey @daxfohl , does DeprecationWarning: isinstance(gate, TwoQubitGate) is deprecated. Use cirq.num_qubits(gate) == 2 instead show up even if i don't call isinstance(gate, TwoQubitGate)?

@daxfohl
Copy link
Collaborator Author

daxfohl commented Sep 1, 2021

It shouldn't. Where are you seeing it?

@vtomole
Copy link
Collaborator

vtomole commented Sep 1, 2021

Oh that's right. Sorry about that. Was confused about something.

@daxfohl
Copy link
Collaborator Author

daxfohl commented Sep 1, 2021

It happens. I just spent half a day trying to figure out why a test framework was rejecting my tests and finally had someone point out that my code wasn't building. :)

rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Closes quantumlib#2164

Copies `Two/ThreeQubitGate` to cirq.testing due to its convenience there, and deprecates the originals.

BREAKING CHANGE: @balopat @95-martin-orion @tanujkhattar During the deprecation period, `isinstance(TwoQubitGate)` has been shimmed such that it will now return True iff _num_qubits_() returns 2, regardless of whether it's actually part of the class hierarchy (and same for ThreeQubitGate). It's not expected that this will cause real-world breakages, but possible.

This can go before or after quantumlib#4236, as they are disjoint problems.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Closes quantumlib#2164

Copies `Two/ThreeQubitGate` to cirq.testing due to its convenience there, and deprecates the originals.

BREAKING CHANGE: @balopat @95-martin-orion @tanujkhattar During the deprecation period, `isinstance(TwoQubitGate)` has been shimmed such that it will now return True iff _num_qubits_() returns 2, regardless of whether it's actually part of the class hierarchy (and same for ThreeQubitGate). It's not expected that this will cause real-world breakages, but possible.

This can go before or after quantumlib#4236, as they are disjoint problems.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Makes googlebot stop complaining. size: L 250< lines changed <1000

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deprecate TwoQubitGate and ThreeQubitGate

8 participants