-
Notifications
You must be signed in to change notification settings - Fork 1k
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
AbstractCircuit.freeze
should not reallocate moments
#5878
Conversation
cirq-core/cirq/circuits/circuit.py
Outdated
@@ -1820,7 +1820,11 @@ def _load_contents_with_earliest_strategy(self, contents: 'cirq.OP_TREE'): | |||
# each index. | |||
for i in range(length): | |||
if i in moments_by_index: | |||
self._moments.append(moments_by_index[i].with_operations(op_lists_by_index[i])) | |||
if op_lists_by_index[i]: | |||
self._moments.append(moments_by_index[i].with_operations(op_lists_by_index[i])) |
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.
Perhaps we could modify Moment.with_operations
instead, so that we can get this optimization in more scenarios, e.g.
def with_operations(self, *contents: 'cirq.OP_TREE') -> 'cirq.Moment':
...
flattened_contents = tuple(op_tree.flatten_to_ops(contents))
if not flattened_contents:
return self
...
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.
Will revert this diff in the next commit,
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.
One comment, then LGTM.
@@ -192,6 +192,9 @@ def with_operations(self, *contents: 'cirq.OP_TREE') -> 'cirq.Moment': | |||
""" | |||
flattened_contents = tuple(op_tree.flatten_to_ops(contents)) | |||
|
|||
if not flattened_contents: | |||
return self |
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.
Please add a unit test of this behavior of Moment.with_operations
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.
Done.
@maffoo Created a big diff compared to the one which was approved. Needs another look. |
These removals don't seem to affect the tests in any way. Don't merge until after #5878
These removals don't seem to affect the tests in any way. Don't merge until after quantumlib#5878
Fixes: #5816