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

Remove implicit assumption that Operations have Gates #3678

Closed
95-martin-orion opened this issue Jan 14, 2021 · 6 comments
Closed

Remove implicit assumption that Operations have Gates #3678

95-martin-orion opened this issue Jan 14, 2021 · 6 comments
Labels
area/gates area/operators area/tech-debt kind/deprecation kind/feature-request Describes new functionality triage/discuss Needs decision / discussion, bring these up during Cirq Cynque

Comments

@95-martin-orion
Copy link
Collaborator

Is your feature request related to a use case or problem? Please describe.

The Operation has a gate property which is frequently used to identify its type. However, not all Operations are GateOperations, meaning that this field is not always populated.

Until recently this has been a relatively minor problem, since the vast majority of Operations are GateOperations. However, with the introduction of CircuitOperation (#3580) this issue is becoming more pronounced.

Related issue: #3235.

Describe the solution you'd like

Remove gate field in Operation and replace with a get_operator abstract method, defined in implementations. This will require deprecating the field and replacing it across the Cirq repo.

[optional] Describe alternatives/workarounds you've considered

  • Add local fixes to sections of code that need to recognize CircuitOperation, and leave the rest unchanged.
    • Issue: eventually, CircuitOperation should be supported everywhere.
  • Make Circuit a subclass of Gate, with CircuitOperations containing CircuitGates
    • Rejected during review (CircuitGates #3509): treating circuits as gates introduces undesirable behavior

What is the urgency from your perspective for this issue? Is it blocking important work?

P1 - I need this no later than the next release (end of quarter)

This confounds work on CircuitOperations, as they do not have a value in their gate field. Continuing work in that space without resolving this issue requires workarounds which will increase the cost of an eventual fix.

@maffoo
Copy link
Contributor

maffoo commented Jan 14, 2021

We don't assume that operations have gates; that's why the gate field has type Optional[cirq.Gate] as opposed to cirq.Gate. Why not just have return None for the gate property on CircuitOperation? I'll admit I'm biased here because I was the one who initially added the gate field (we explicitly made it optional to accomodate all operations). This change got rid of helper functions that we used previously to identify GateOperations and extract their gates and check the gate type, etc. and simplified a lot of code so I'd prefer to keep it.

@95-martin-orion
Copy link
Collaborator Author

Why not just have return None for the gate property on CircuitOperation?

This is the current behavior for CircuitOperation. If we keep this behavior, every line of code that relies on op.gate to recognize the type of an operation must separately check for CircuitOperation - which itself is awkward, because simply checking isinstance(op, CircuitOperation) fails to identify CircuitOperation(...).with_tags(...), which is a TaggedOperation.

Separately, this is an example of the interface segregation principle: Operations that are not GateOperations should not be required to have a gate field, even if that field defaults to None.

The example of this which prompted me to open this issue was serializable_gate_set.py, which uses op.gate to choose which serializer to use.

@daxfohl
Copy link
Contributor

daxfohl commented Jan 16, 2021

It may be worth revisiting whether a subcircuit should really be modeled as an operation. An operation is plausibly something that's instantaneous, that plays out in a single moment. A subcircuit spans moments and runs in sequence in sync with the main circuit.

Either way, seems like there should be something in the class hierarchy that distinguishes between instantaneous and sequential. Maybe an algebraic data type would be better here than subclassing?

@95-martin-orion
Copy link
Collaborator Author

95-martin-orion commented Jan 19, 2021

@daxfohl: Subcircuits as they are implemented today are just as "instantaneous" as any other operation. Consider this circuit:

a, b, = cirq.LineQubit.range(2)
circuit = cirq.Circuit(
  cirq.Moment(
    cirq.H(a),
    cirq.H(b),
  ),
  cirq.Moment(
    cirq.measure(a, key='m1'),
    cirq.CircuitOperation(
      cirq.FrozenCircuit(
        cirq.Moment(cirq.T(b)),
        cirq.Moment(cirq.H(b)),
        cirq.Moment(cirq.X(b)**0.5),
      )
    ),
  ),
  cirq.Moment(
    cirq.X(a),
    cirq.measure(b, key='m2'),
  ),
)

There are two things to note about the second moment of the top-level circuit:

  • The CircuitOperation runs entirely within the second moment, alongside the measurement. It will not "spill over" into the third moment - the moments it contains are "sub-moments" of the second top-level moment, in a sense.
  • On many types of quantum hardware, the measure operation will actually take longer than the CircuitOperation, despite being a single operation. Hardware durations of operations can vary considerably.

(These items are true even without explicit cirq.Moments wrapping each moment; I just added those to clarify the structure.)

For more details on why CircuitOperation was implemented as it is, you can read the original RFC here: https://tinyurl.com/cirq-circuitoperation-rfc.

@daxfohl
Copy link
Contributor

daxfohl commented Feb 3, 2021

Create a module-level function ops.try_get_gate(op: Operation)? That way it's still a one-liner but it fixes ISP and is obvious it can fail.

CirqBot pushed a commit that referenced this issue Mar 9, 2021
Since `Operation` provides a canonical way to get the untagged version of itself, we should use it instead of writing a new method to do the same thing.

Using `op.untagged` in #3634 removes any residual blocking effects from #3678, although I'm still in favor of finding a proper resolution to #3678.
@mpharrigan
Copy link
Collaborator

  • Violates some software engineering principles
  • Changing it would be too hard
  • @95-martin-orion says there's more tools now so he's less annoyed.

Short term solution: #3893
Long term solution: #2536

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gates area/operators area/tech-debt kind/deprecation kind/feature-request Describes new functionality triage/discuss Needs decision / discussion, bring these up during Cirq Cynque
Projects
None yet
Development

No branches or pull requests

4 participants