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

How to determine if an operation has a gate? #3556

Closed
dabacon opened this issue Dec 1, 2020 · 11 comments
Closed

How to determine if an operation has a gate? #3556

dabacon opened this issue Dec 1, 2020 · 11 comments
Labels
area/tags area/transformers kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on triage/discuss Needs decision / discussion, bring these up during Cirq Cynque

Comments

@dabacon
Copy link
Collaborator

dabacon commented Dec 1, 2020

It used to be that you could check whether an operation had a gate using a type check
isinstance(gate_op, cirq.GateOperation)
But if you tag an op it loses that signature.
isinstance(gate_op.with_tags('you are it'), cirq.GateOperation) == False

Note that there are a lot of these checks throughout Cirq that will presumably break when you tag an operation. For example look at cirq.EjectPhasedPaulis().

eject = cirq.EjectPhasedPaulis()
q = cirq.NamedQubit('a')
circuit = cirq.Circuit(cirq.PhasedXPowGate(phase_exponent=0.125)(q), cirq.Z(q))
eject.optimize_circuit(circuit)
print(circuit)

does the correct optimization

a: ───PhX(0.625)───────

Put if you tag the Z, it doesn't do the optimization

circuit = cirq.Circuit(cirq.PhasedXPowGate(phase_exponent=0.125)(q), cirq.Z(q).with_tags('you are it')
eject.optimize_circuit(circuit)
print(circuit)

prints

a: ───PhX(0.125)───Z['you are it']───
@balopat balopat added area/transformers area/tags kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on labels Dec 1, 2020
@balopat
Copy link
Contributor

balopat commented Dec 1, 2020

Thanks for opening this!

Given that this breaks the optimizers I consider this a bug.
Also cross ref #2536 - could be that a generic pattern matching will be the solution here.

@maffoo
Copy link
Contributor

maffoo commented Dec 1, 2020

cirq.Operation has a gate property of type Optional[Gate] so you can check it against None on any operation:

if op.gate is not None:
    ...

Note that TaggedOperation delegates gate to the underlying operation (https://github.com/quantumlib/Cirq/blob/master/cirq/ops/raw_types.py#L534) so it also works fine to access the gate of a tagged operation directly without going through the sub operation.

@maffoo
Copy link
Contributor

maffoo commented Dec 1, 2020

If there are still places that do isinstance checks against GateOperation we should change them to use the gate property instead.

@95-martin-orion
Copy link
Collaborator

A similar issue exists for CircuitOperations (#3580), which do not populate the gate field but instead have a circuit field not present in the base Operation class.

@95-martin-orion
Copy link
Collaborator

A similar issue exists for CircuitOperations {...}

Opened #3678 to track this. As noted on that issue, I think we should remove the gate field from the base Operation class, since not all Operations are GateOperations.

@mpharrigan
Copy link
Collaborator

We had not-that-long-ago added gate to Operation because the proliferation of isinstance(op, GateOperation) was annoying

@95-martin-orion
Copy link
Collaborator

We had not-that-long-ago added gate to Operation because the proliferation of isinstance(op, GateOperation) was annoying

Acknowledged - my recommendation on the linked issue is to replace the gate field with a more generic get_operator method that accomplishes the same purpose, but with flexible support for other Operation subtypes.

CirqBot pushed a commit that referenced this issue Oct 12, 2021
…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.
@dstrain115 dstrain115 added the triage/discuss Needs decision / discussion, bring these up during Cirq Cynque label Jun 6, 2022
@dstrain115
Copy link
Collaborator

Marking this as triage discuss, so that we can decide on a strategy for what to do with this (change everything to .gate or use get_operator)

@95-martin-orion
Copy link
Collaborator

As noted on the (now closed) #3678, I've given up on get_operator. If there are still operation types that don't have a gate field we can track that here, but otherwise I think it's fine to close.

@tanujkhattar
Copy link
Collaborator

+1, I think op.gate should be the way to go. We tracked in #4683 that all operations (except circuit operations and ccos) should have an underlying op.gate; so we can close this issue now and open a new issue if we find operations that should but don't have an underlying gate property.

@dstrain115
Copy link
Collaborator

As per above comments, we should use op.gate and all operations should have a gate. Closing this issue.

rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tags area/transformers kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on triage/discuss Needs decision / discussion, bring these up during Cirq Cynque
Projects
None yet
Development

No branches or pull requests

8 participants