-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Repetition IDs for CircuitOperation #3741
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
Conversation
cirq/circuits/circuit_operation.py
Outdated
repetition_ids: List of IDs, one for each repetition. The length | ||
should be equal to either `repetitions` or | ||
`repetitions*self.repetitions`. In the latter case, these |
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.
What is the use case for the repetitions * self.repetitions
/ override-all-IDs behavior? It seems like the repetitions
/ prefixing case should be sufficient.
Separately, we shouldn't rely on the size of this argument to determine its purpose. If we need both behaviors, we should have a separate argument (or method) for each.
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.
Since we assign default repetition_ids
even when the circuit is non-measurement, without override-all-IDs the following will be true
circuit_op.repeat(2).repeat(3) != circuit_op.repeat(6)
circuit_op.repeat(2).repetition_ids == ['0-0', '1-0']
circuit_op.repeat(2, ['a', 'b']).repetition_ids == ['a-0', 'b-0']
The above can be handled by special casing based on if the base or input repetition_ids are "defaults"/None. But if we are overriding in some cases, might as well make that behavior available to the caller. For e.g. if circuit_op.repeat(2).repeat(3) == circuit_op.repeat(6)
, it would be nice to be able to specify all 6 repetition_ids
in the left-side construction.
But I agree that size-based might not be the best way to go about this. How about I add an override_all_ids: Optional[bool]
arg? If specified, the repetition_ids
size will be validated based on that. If unspecified, we choose the default value of override_all_ids
based on repetition_ids
and self.repetition_ids
.
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.
Made the above modifications. Let me know if this looks better.
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.
In my mind, op.repeat(2).repeat(3) != op.repeat(6)
is the correct behavior, since they represent different things. Conceptually, it's similar to how [[0, 1], [2, 3]] != [0, 1, 2, 3]
.
Regarding circuit_op.repeat(2).repetition_ids
: if the original circuit_op
has an explicitly-defined ID of 0
for its single repetition, I think ['0-0', '1-0']
is correct. However, if no ID has been defined, we want ['0', '1']
instead. This behavior is similar to a raw value vs. a list with one element:
a = 0
a2 = [a, a] # [0, 0]
a2[0] == a2[1] == 0
b = [0]
b2 = [b, b] # [[0], [0]]
b2[0][0] == b2[1][0] == 0
With this in mind, our only edge case is the single-repetition, no-ID CircuitOperation; everything else follows the "prefix" case. By extension, we can drop override_all_ids
and the extra complexity associated with it.
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.
Sure, I was more going for the ControlledGate
type behavior we have. Chained controls with different number of qubits are automatically "flattened" and the final ControlledGate
is equivalent to if all of them were applied at once.
But I guess pow
semantics (repeat) don't need to be similar to that addition
type operation.
Made the above change and simplified a couple of other things.
cirq/circuits/circuit_operation.py
Outdated
) | ||
|
||
# Methods for constructing a similar object with one field modified. | ||
|
||
def repeat( | ||
self, | ||
repetitions: INT_TYPE, | ||
self, repetitions: INT_TYPE, repetition_ids: Optional[List[str]] = None |
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.
What do you think about making both args optional? I think it'd be nice for repeat(['a', 'b', 'c'])
to work without specifying the number of IDs. (Requires addressing repetitions
vs repetitions * self.repetitions
first)
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 was what I had originally implemented but because of multiple optionals, the default-ing logic was becoming too complicated and I felt it was making the API messier. But if we are making the override-all-IDs case more explicit, I should be able to add this in as well.
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.
Made repetitions
Optional
as well. The actual conditions of what defaults to what are a little difficult to describe but added lot of docstrings to compensate. Also, even though it's verbose to explain, the actual behavior seems pretty intuitive to me.
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.
Now it's even more intuitive :)
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.
A few relatively minor items and this should be good to go.
Useful for keying individual measurements in repeated
CircuitOperation
sCloses #3663