Skip to content
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.

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

Move on_each to cirq.Gate #4236

Closed
wants to merge 26 commits into from
Closed

Move on_each to cirq.Gate #4236

wants to merge 26 commits into from

Conversation

daxfohl
Copy link
Contributor

@daxfohl daxfohl commented Jun 21, 2021

Part of #4034. (See #4034 (comment))

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

Note that deprecating SingleQubitGate will be more troublesome than the others, as things like _CALIBRATION_IRRELEVANT_GATES and NOISE_MODEL_LIKE depend on it.

@daxfohl daxfohl requested review from cduck, vtomole and a team as code owners June 21, 2021 00:58
@daxfohl daxfohl requested a review from mpharrigan June 21, 2021 00:58
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Jun 21, 2021
This was referenced Jun 21, 2021
Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

I don't think this follows the proposed behavior. Hopefully restricting to either 1) qubits for one qubit gate or 2) iterables of tuples for multiq gates will permit a simpler implementation as well. It's rather involved right now

cirq-core/cirq/ops/gate_features.py Show resolved Hide resolved
cirq-core/cirq/ops/raw_types.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/raw_types.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/raw_types.py Outdated Show resolved Hide resolved
@95-martin-orion
Copy link
Collaborator

@daxfohl is this ready for re-review?

@daxfohl
Copy link
Contributor Author

daxfohl commented Jul 1, 2021

Yes, the new code only supports the strict option for multi-qubit gates, per the discussion here: #4034 (comment)

@maffoo
Copy link
Contributor

maffoo commented Jul 1, 2021

@daxfohl, how would you feel about splitting this into two PRs, one to generalize SupportsOnEachGate to handle >1 qubits, and then a second PR to move on_each into the Gate base class?

cirq-core/cirq/ops/gate_features_test.py Show resolved Hide resolved
cirq-core/cirq/ops/raw_types.py Show resolved Hide resolved
cirq-core/cirq/ops/raw_types.py Show resolved Hide resolved
cirq-core/cirq/ops/raw_types.py Show resolved Hide resolved
@mpharrigan
Copy link
Collaborator

some readability concerns raised above. I agree with @maffoo that this change would benefit from being split into the two proposed parts

@daxfohl
Copy link
Contributor Author

daxfohl commented Jul 1, 2021

Sure, I can address those concerns and split into two PRs.

@daxfohl
Copy link
Contributor Author

daxfohl commented Jul 2, 2021

First part added at #4281. We can return to this later then. I'll move it to draft status. @mpharrigan @maffoo

@daxfohl daxfohl marked this pull request as draft July 2, 2021 06:07
CirqBot pushed a commit that referenced this pull request Jul 9, 2021
Adds on_each support to the SupportsOnEachGate mixin for multi-qubit gates.

The handling here is not as flexible as for single-qubit gates, which allows any tree of gates and applies them depth-first. This allows the following two options for multi-qubit gates:

```
A: varargs form gate.on_each([q1, q2], [q3, q4])
B: explicit form gate.on_each([[q1, q2], [q3, q4]])
```


Discussion here, #4034 (comment). Part of #4236.
@daxfohl
Copy link
Contributor Author

daxfohl commented Jul 9, 2021

Closing in favor of #4303

@daxfohl daxfohl closed this Jul 9, 2021
@daxfohl daxfohl deleted the oneach branch July 13, 2021 13:34
CirqBot pushed a commit that referenced this pull request Aug 31, 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.
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Adds on_each support to the SupportsOnEachGate mixin for multi-qubit gates.

The handling here is not as flexible as for single-qubit gates, which allows any tree of gates and applies them depth-first. This allows the following two options for multi-qubit gates:

```
A: varargs form gate.on_each([q1, q2], [q3, q4])
B: explicit form gate.on_each([[q1, q2], [q3, q4]])
```


Discussion here, quantumlib#4034 (comment). Part of quantumlib#4236.
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.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants