-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add LATEST InsertStrategy to complement EARLIEST #7804
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
base: main
Are you sure you want to change the base?
Conversation
Scans forward from a given insert location and inserts an operation into the latest possible moment. If no moment is available, inserts the operation in a new moment.
|
I updated Handling a single operation is rather straightforward. However,
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7804 +/- ##
=======================================
Coverage 99.57% 99.57%
=======================================
Files 1102 1102
Lines 98638 98710 +72
=======================================
+ Hits 98214 98290 +76
+ Misses 424 420 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
NoureldinYosri
left a comment
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.
LGTM but I would like @pavoljuhas to take a look since this is a big change to circuit construction
| the right is available, or `len(self._moments)` if `start_moment_index` equals it. | ||
| """ | ||
| if start_moment_index is None: | ||
| start_moment_index = 0 |
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.
why note set the default to zero ?
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.
+1 Since _latest_available_moment is an internal method with only one in-class caller, let us make the start_moment_index argument mandatory.
| if moment.operates_on(op_qubits): | ||
| return k - 1 | ||
| moment_measurement_keys = moment._measurement_key_objs_() | ||
| if ( |
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.
[super optional] nit: turn the not a or not b or not c into not (a and b and c)
pavoljuhas
left a comment
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.
LGTM with a couple of smallish adjustments, please see inline comments.
Thank you for taking care of this!
| the right is available, or `len(self._moments)` if `start_moment_index` equals it. | ||
| """ | ||
| if start_moment_index is None: | ||
| start_moment_index = 0 |
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.
+1 Since _latest_available_moment is an internal method with only one in-class caller, let us make the start_moment_index argument mandatory.
| def _insert_latest(self, k: int, batches: list[list[_MOMENT_OR_OP]]) -> int: | ||
| """Inserts batches of moments or operations using LATEST strategy. | ||
| Batches are inserted into reverse order. |
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.
| Batches are inserted into reverse order. | |
| Batches are processed in reverse order. |
| batches = batches[::-1] | ||
| max_latest_index = -1 # Maximum index of a changed moment | ||
| for batch in batches: |
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.
LGTM with a couple of suggestions:
-
the corner case
c.insert(index, [])returnsindexwith the EARLIEST strategy, but 0 with the LATEST. Let us make this consistent, ie, returnindexfor either strategy. Please also add a test for this case. -
I found it a bit tricky to follow the
_insert_latestcode with Moment and Operation blocks interspersed. Would you mind applying the patch below which regroups them to (a) Moment-only and (b) Operation only blocks?
diff --git a/cirq-core/cirq/circuits/circuit.py b/cirq-core/cirq/circuits/circuit.py
index 74c08354..6ecfcb41 100644
--- a/cirq-core/cirq/circuits/circuit.py
+++ b/cirq-core/cirq/circuits/circuit.py
@@ -2214,5 +2214,4 @@ class Circuit(AbstractCircuit):
"""
- batches = batches[::-1]
max_latest_index = -1 # Maximum index of a changed moment
- for batch in batches:
+ for batch in reversed(batches):
for moment_or_op in batch:
@@ -2220,22 +2219,22 @@ class Circuit(AbstractCircuit):
if isinstance(moment_or_op, Moment):
- p = k
+ self._moments.insert(k, moment_or_op)
+ max_latest_index = max(k, max_latest_index + 1)
else:
+ end_moment_index = len(self.moments)
p = self._latest_available_moment(moment_or_op, start_moment_index=k)
if p < k:
- self._moments.insert(k, Moment())
- # All later moments shift by 1, so increase the max index
- max_latest_index += 1
- p = k
- # Place
- if isinstance(moment_or_op, Moment):
- self._moments.insert(p, moment_or_op)
- # All later moments shift by 1, so increase the max index
- max_latest_index += 1
- elif p == len(self._moments):
- self._moments.append(Moment(moment_or_op))
- else:
- self._moments[p] = self._moments[p].with_operation(moment_or_op)
- max_latest_index = max(p, max_latest_index)
- self._mutated(preserve_placement_cache=False)
- return max_latest_index + 1
+ self._moments.insert(k, Moment.from_ops(moment_or_op))
+ max_latest_index = max(k, max_latest_index + 1)
+ elif p < end_moment_index:
+ self._moments[p] = self._moments[p].with_operation(moment_or_op)
+ max_latest_index = max(p, max_latest_index)
+ else:
+ assert p == end_moment_index
+ self._moments.append(Moment.from_ops(moment_or_op))
+ max_latest_index = end_moment_index
+ # handle returned position index for empty batch
+ pos = k if max_latest_index == -1 else max_latest_index + 1
+ if max_latest_index != -1:
+ self._mutated(preserve_placement_cache=False)
+ return pos
Scans forward from a given insert location and inserts an operation into the latest possible moment. If no moment is available, inserts the operation in a new moment.
Fixes #7611