CircuitOperation: change use_repetition_ids default to False #6910
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6910 +/- ##
=======================================
Coverage 97.86% 97.86%
=======================================
Files 1084 1084
Lines 94290 94299 +9
=======================================
+ Hits 92280 92289 +9
Misses 2010 2010 ☔ View full report in Codecov by Sentry. |
| @@ -617,7 +624,12 @@ def repeat( | |||
|
|
|||
| # The eventual number of repetitions of the returned CircuitOperation. | |||
| final_repetitions = protocols.mul(self.repetitions, repetitions) | |||
| return self.replace(repetitions=final_repetitions, repetition_ids=repetition_ids) | |||
| print(f"repeat: {repetitions=}. {repetition_ids=}, {use_repetition_ids=}") | |||
There was a problem hiding this comment.
Intentional or a debug print out?
There was a problem hiding this comment.
Ah yes, this was unintentional. Removed.
| use_repetition_ids: bool = True, | ||
| use_repetition_ids: Optional[bool] = None, |
There was a problem hiding this comment.
Nit - consider updating the use_repetition_ids docstring below.
| @@ -577,6 +577,10 @@ def repeat( | |||
| defaults to the length of `repetition_ids`. | |||
| repetition_ids: List of IDs, one for each repetition. If unset, | |||
| defaults to `default_repetition_ids(repetitions)`. | |||
| use_repetition_ids: If given, this specifies the value for `use_repetition_ids` | |||
| of the resulting circuit operation. If the not given, we enable | |||
There was a problem hiding this comment.
| of the resulting circuit operation. If the not given, we enable | |
| of the resulting circuit operation. If not given, we enable |
| [ [ a: ═══@═══^═══ ](loops=2) ](loops=2) | ||
| [ [ 0: ───M───X─── ] ] | ||
| 0: ───[ 0: ───[ ║ ║ ]────────────────────────────── ]────────────────────────────── | ||
| [ [ a: ═══@═══^═══ ](repetition_ids=['0', '1']) ](repetition_ids=['0', '1']) |
There was a problem hiding this comment.
This might get quite long for large repetition count.
Should we perhaps use (loops=2, use_repetition_ids=True) instead?
PS: No strong preference on my side.
There was a problem hiding this comment.
Agreed. Changed to use (loops=2, use_repetition_ids=True) as suggested.
This changes
CircuitOperationso thatuse_repetition_idsdefaults to False rather than True. If explicitrepetition_idsare provided (to the constructor or to thereplacemethod), we setrepetition_idsto True automatically. The motivation for this is that internally we have never used repetition_ids and in fact we have helper methods for creating looped circuit operations since it is easy to forget to passuse_repetition_ids=Falseto override the default. The history here is that we started implementing repetition_ids at about the same time as we started implementing support for repeated measurement keys, and unfortunately I think we picked the wrong default for the keys. All of our internal experiments such as QEC and looped sampling experiments have setuse_repetition_ids=Falsebecause repeated keys make it much more efficient to retrieve and manipulate data (one can get a single 3D array of bits for the repeated key instead of separate 2D arrays for each key instance with a different id) and so this better aligns cirq defaults with actual usage.