Adding tags to Moments #7467
Adding tags to Moments #7467
Conversation
dstrain115
commented
Jul 2, 2025
dstrain115
commented
- This PR adds the ability to add tags to Moment objects.
- tags in Moments function similar to tags in operations, in that they can be any hashable object and are lost if the circuit undergoes transformation, such as by a transformer.
- This PR adds the ability to add tags to Moment objects. - tags in Moments function similar to tags in operations, in that they can be any hashable object and are lost if the circuit undergoes transformation, such as by a transformer.
| @@ -183,7 +216,7 @@ def with_operation(self, operation: cirq.Operation) -> cirq.Moment: | |||
| raise ValueError(f'Overlapping operations: {operation}') | |||
|
|
|||
| # Use private variables to facilitate a quick copy. | |||
| m = Moment(_flatten_contents=False) | |||
| m = Moment(_flatten_contents=False, tags=self._tags) | |||
There was a problem hiding this comment.
PR description says tags are lost if the moment undergoes transformation, but here tags are retained. Either way is probably fine, but wanted to make sure this change was intentional.
There was a problem hiding this comment.
+1 - perhaps it would be simpler to remove tags anytime we return a new Moment with changed operations. This should apply to the add / sub operators below as well. A way to think of it is that tags were given to a moment with some desired behavior / operations. If that changes, the tags should be dropped.
Currently there is an inconsistency that with_operation preserves tags, but without_operations_touching removes them.
Or is there some requirement that tags should be sticky and survive some Moment transformations?
There was a problem hiding this comment.
Depends on definition of "simpler". If Moment was a dataclass, which I think there's a decent argument that it should be, it'd be simpler to leave them.
That said, I think it's understandable if the with_ methods here leave them, but transformers remove them (which I think is what the PR description was actually referring to), since transformers reorganize the whole circuit in ways that moments are not preserved.
So, I don't have much of a preference whether the with_ methods here preserve tags or not (okay maybe a slight preference that they do preserve them--I think it's what users would expect, and the workaround for users to re-add tags after each with_ if we're not preserving them, is more troublesome than the workaround for the opposite, which they could do via moment.with_whatever(...).without_tags()). But either way is fine. The important thing is that whichever way we go, the methods on the Moment class itself should handle them consistently.
| self, | ||
| *contents: cirq.OP_TREE, | ||
| _flatten_contents: bool = True, | ||
| tags: tuple[Hashable, ...] = (), |
There was a problem hiding this comment.
Should we add a docstring for tags or is it intentionally hidden?
| @@ -110,6 +116,7 @@ def __init__(self, *contents: cirq.OP_TREE, _flatten_contents: bool = True) -> N | |||
|
|
|||
| self._measurement_key_objs: frozenset[cirq.MeasurementKey] | None = None | |||
| self._control_keys: frozenset[cirq.MeasurementKey] | None = None | |||
| self._tags = tags | |||
|
|
|||
| @classmethod | |||
| def from_ops(cls, *ops: cirq.Operation) -> cirq.Moment: | |||
There was a problem hiding this comment.
Let us add an optional tags argument to from_ops too.
| Please note that tags should be instantiated if classes are | ||
| used. Raw types are not allowed. | ||
| """ | ||
| return Moment(*self._operations, _flatten_contents=False, tags=(*self._tags, *new_tags)) |
There was a problem hiding this comment.
Nit - consider adding a shortcut if not new_tags: return self
| cirq.Moment( | ||
| cirq.X(cirq.LineQubit(0)), | ||
| tags=cirq.Duration(nanos=25) |
There was a problem hiding this comment.
This does not fit the declared tuple[Hashable] type.
Please fix here and in the example json.
