-
Notifications
You must be signed in to change notification settings - Fork 983
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
Fix CCO related nits in cirq.Operation
and cirq.TaggedOperation
#5390
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overarching comment: prefer if not some_list
instead of if len(some_list) == 0
.
Otherwise this looks good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@95-martin-orion Made a couple of changes to make the tests pass. PTAL and I'll merge once you re-approve.
@@ -385,7 +385,7 @@ def test_condition_removal(): | |||
op = op.without_classical_controls() | |||
assert not cirq.control_keys(op) | |||
assert not op.classical_controls | |||
assert set(map(str, op.tags)) == {'t1'} | |||
assert not op.tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@95-martin-orion This changes the assumption that tags are preserved when calling with_classical_controls
. This looks reasonable to me; since even in the original version, only t1
was preserved instead of both t1
and t2
.
Just want to make sure this looks good to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstrings for with/without_classical_controls both describe how tags are handled. If that behavior needs to change then the docstrings need updated to be consistent. (Probably can just change "hide" to "remove" in with
, and I guess you can delete the blurb in without
completely since it's no longer applicable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to Dax's comment, other than that I'm on board. Might want a BREAKING_CHANGE label to keep track of this, but I see it as a bugfix - the intent is for any non-tagging operation to eliminate tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the docstring.
@@ -864,6 +868,13 @@ def without_classical_controls(self) -> 'cirq.Operation': | |||
new_sub_operation = self.sub_operation.without_classical_controls() | |||
return self if new_sub_operation is self.sub_operation else new_sub_operation | |||
|
|||
def with_classical_controls( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this logic go in the CCO constructor instead? Otherwise, users will still be able to do cirq.ClassicallyControlledOperation(cirq.TaggedOperation(...))
and still have those tags retained inside the CCO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine, and also consistent with other operations like controlled ops. The general guidance would also be to use op.with_classical_controls
instead of using the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg, I guess the without
docstring doesn't need updated then, unless there's a need to be explicit that that's the only case in which a TaggedOp could be returned. (with
still needs updated though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback.
Merging now to unblock the next PR, which will fix #4977
cirq-core/cirq/ops/raw_types.py
Outdated
If no conditions are specified, returns self. | ||
|
||
The classical control will remove any tags on the existing operation, | ||
since tagged operations are considered to be immutable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason isn't that TaggedOperations are immutable, as this creates a new operation - no mutation is taking place regardless. The actual reason is that tags are fragile - for an arbitrary tag, it's not possible to determine whether it should be preserved after a change, so we opt to always discard them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's why I tried writing "considered" to be immutable.
How about I reword it to?
The classical control will remove any tags on the existing operation,
since tags are fragile, and we always opt to get rid of the tags when
the underlying operation is changed.
Any suggestions with the right wording are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good to me. Granting approval with the assumption that this gets applied.
…uantumlib#5390) * Improve CCO support in cirq.Operation and cirq.TaggedOperation * Fix failing tests * Update docstrings * Clarify without_classical_controls docstring * Reword docstrings
Asserts that:
op.with_classical_controls() is op
: Returns self if set of conditions is empty. This is already true for other methods likeop.with_tags
,op.controlled_by
etc.tagged_op.with_classical_controls()
will now remove tags, which is consistent with the philosophy that tagged operations are immutable and tags should be removed if the underlying operation is mutated. Other methods, liketagged_op.controlled_by
, already follow this pattern.