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

Replace _get_op_circuit with op.untagged #3893

Merged
merged 3 commits into from
Mar 9, 2021
Merged

Replace _get_op_circuit with op.untagged #3893

merged 3 commits into from
Mar 9, 2021

Conversation

95-martin-orion
Copy link
Collaborator

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.

@95-martin-orion 95-martin-orion requested review from cduck, vtomole and a team as code owners March 9, 2021 14:48
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Mar 9, 2021
@@ -776,13 +776,13 @@ def are_all_matches_terminal(self, predicate: Callable[['cirq.Operation'], bool]
if not all(
self.next_moment_operating_on(op.qubits, i + 1) is None
for (i, op) in self.findall_operations(predicate)
if _get_op_circuit(op) is None
if getattr(op.untagged, 'circuit', None) is None
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would more clearly convey intent here to check the type instead

Suggested change
if getattr(op.untagged, 'circuit', None) is None
if not isinstance(op.untagged, CircuitOperation)

Would have to import CircuitOperation in are_all_matches_terminal and are_any_matches_terminal, but that should be fine. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seems reasonable to me. Change applied (+ imports)

Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

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

LGTM

@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Mar 9, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Mar 9, 2021
@CirqBot CirqBot merged commit d4f3fc9 into master Mar 9, 2021
@CirqBot CirqBot deleted the get-op-circuit branch March 9, 2021 17:15
@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 Mar 9, 2021
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