Fix CCO related nits in cirq.Operation and cirq.TaggedOperation#5390
Conversation
95-martin-orion
left a comment
95-martin-orion
left a comment
There was a problem hiding this comment.
Overarching comment: prefer if not some_list instead of if len(some_list) == 0.
Otherwise this looks good to go.
tanujkhattar
left a comment
tanujkhattar
left a comment
There was a problem hiding this comment.
@95-martin-orion Made a couple of changes to make the tests pass. PTAL and I'll merge once you re-approve.
| 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.
@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.
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.
+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.
Updated the docstring.
| 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.
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.
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.
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).
tanujkhattar
left a comment
tanujkhattar
left a comment
There was a problem hiding this comment.
Thanks for the feedback.
Merging now to unblock the next PR, which will fix #4977
| 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.
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.
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.
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
…uantumlib#5390) * Improve CCO support in cirq.Operation and cirq.TaggedOperation * Fix failing tests * Update docstrings * Clarify without_classical_controls docstring * Reword docstrings
4 participants
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_byetc.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.