Add Tags and TaggedOperation class #2670
Conversation
Draft of basic Operator Tag functionality. - Tags are a wrapper around strings that allow you to mark a specific instance of an operator for special processing. - This can be used, for example, to exclude certain operations from optimizer routines or give operations information specific to the underlying hardware. This PR adds a Tag class which is a wrapper around a string and can also be sub-classed. It also adds a TaggedOperation class which wraps an Operation and includes Tags on that Operation.
|
Adding an additional reviewer. I'd love to get this reviewed this week. Thanks! |
cirq/ops/raw_types.py
Outdated
| return protocols.trace_distance_bound(self.sub_operation) | ||
|
|
||
| def _phase_by_(self, phase_turns: float, | ||
| qubit_index: int) -> 'TaggedOperation': |
There was a problem hiding this comment.
The return type here is TaggedOperation, but the method doesn't appear to preserve the tags.
There was a problem hiding this comment.
Done. The intended behavior is that tagging only applies to that specific operation. Any modifications should drop the tags. This is probably the only way to have a consistent behavior.
For that reason, I have changed the return type to Operation. I also modified controlled_by, which was not following this convention.
cirq/ops/raw_types.py
Outdated
| @@ -392,6 +393,24 @@ def with_qubits(self, *new_qubits: 'cirq.Qid') -> 'cirq.Operation': | |||
| `qubits` property. | |||
| """ | |||
|
|
|||
| def with_tags(self, *new_tags: Any) -> 'cirq.TaggedOperation': | |||
There was a problem hiding this comment.
Should TaggedOperation override this to add new_tags to itself (or replace its tags with new_tags), instead of nesting it in another TaggedOperation?
There was a problem hiding this comment.
Good idea. I added a with_tags() on TaggedOperation that avoids nesting.
cirq/ops/raw_types.py
Outdated
|
|
||
| def __init__(self, sub_operation: 'cirq.Operation', *tags: Any): | ||
| self.sub_operation = sub_operation | ||
| self._tags = list(tags) |
There was a problem hiding this comment.
Why convert it into a list when it's already a tuple? Tuple guarantees no shenanigans.
cirq/ops/raw_types.py
Outdated
| See Operation.with_tags() for more information on intended usage. | ||
| """ | ||
|
|
||
| def __init__(self, sub_operation: 'cirq.Operation', *tags: Any): |
There was a problem hiding this comment.
Should tags be required to be Hashable or even JSON serializable?
There was a problem hiding this comment.
Changed to Hashable and put a warning in the pydoc to make tags JSON serialiable if you want to serialize the circuit. I don't think JSON serializability should be a strict requirement, but it's nice to remember.
cirq/ops/raw_types.py
Outdated
| return str(self) | ||
|
|
||
| def _value_equality_values_(self): | ||
| return (self.sub_operation, *self._tags) |
There was a problem hiding this comment.
This seems inefficient for a type that may be hashed often.
There was a problem hiding this comment.
Why would it be less efficient than just the sub_operation plus tags?
There was a problem hiding this comment.
I was thinking to avoid creating a new tuple of length 1+len(tags) by nesting tuples instead of flattening. Now that I think about it, there won't usually be a large number of tags so it really doesn't matter.
# in __init__
self._tags = tuple(...)
def _value_equality_values_(self):
return (self.sub_operation, self._tags)
There was a problem hiding this comment.
Ah, I was also confused about what you were concerned about. I have changed it to a tuple, though I doubt the performance will matter too much.
I felt that was tricky because it raises the question of what a basic operation is, and when does or doesn't this apply. Making it so that any new creation deletes tags is sometimes annoying, but never ambiguous. |
cirq/ops/raw_types.py
Outdated
| control_values=control_values) | ||
|
|
||
| @property | ||
| def tags(self): |
cirq/ops/raw_types.py
Outdated
| """Returns a tuple of the operation's tags.""" | ||
| return self._tags | ||
|
|
||
| def with_tags(self, *new_tags: Any) -> 'cirq.TaggedOperation': |
There was a problem hiding this comment.
Change to Hashable here as well.
… into tagged_operation
Draft of basic Operator Tag functionality.
mark a specific instance of an operator for special processing.
from optimizer routines or give operations information specific
to the underlying hardware.
This PR adds a Tag class which is a wrapper around a string and
can also be sub-classed.
It also adds a TaggedOperation class which wraps an Operation and
includes Tags on that Operation.