Replace isinstance(op, GateOperation) checks in cirq_google optimizers to support other operation types.
#4459
Conversation
tanujkhattar
requested review from
cduck,
vtomole,
wcourtney and
a team
as code owners
…ace_isinstance_google
…hattar/Cirq into replace_isinstance_google
There was a problem hiding this comment.
I'm not sure we have the luxury of considering non-compilation of TaggedOperation to be a bug, since we've previously advertised this as a way to prevent operations from compiling. Could you confirm with users (e.g. @maffoo, @mpharrigan) that this isn't being used outside of Cirq?
cirq-google/cirq_google/optimizers/convert_to_sycamore_gates.py
Outdated
Show resolved
Hide resolved
| @@ -139,7 +139,7 @@ def on_stuck_raise(bad): | |||
| def optimization_at( | |||
| self, circuit: cirq.Circuit, index: int, op: cirq.Operation | |||
| ) -> Optional[cirq.PointOptimizationSummary]: | |||
| if not isinstance(op, cirq.GateOperation): | |||
| if op.gate is None: | |||
There was a problem hiding this comment.
[No change needed] #4511 will modify this to support CircuitOperations, whose gate is None. Are we still okay with using isinstance to identify CircuitOperations after this PR?
There was a problem hiding this comment.
No, we should not use isinstance to identify CircuitOperations, or else we will get into the same problem. For example, a controlled / tagged CircuitOperation would not get identified if use isinstance checks.
cirq-google/cirq_google/optimizers/convert_to_xmon_gates_test.py
Outdated
Show resolved
Hide resolved
cirq-google/cirq_google/optimizers/convert_to_sycamore_gates_test.py
Outdated
Show resolved
Hide resolved
tanujkhattar
added
the
BREAKING CHANGE
…ace_isinstance_google
|
@95-martin-orion As discussed in the Cirq Sync, this is indeed a bug and should be fixed. I've addressed the remaining nit's and the PR is ready for re-review. |
tanujkhattar
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
…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.
Proliferation of
isinstance(op, GateOperation)checks results in many inconsistencies due to different available operation types likeControlledOperationsandTaggedOperations. This PR fixes #4152 and is a first step towards fixing #3556Note that
TaggedOperationswhich 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 explicitNoCompiletag should be implemented as part of #4253This 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.