-
Notifications
You must be signed in to change notification settings - Fork 984
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
Make moment str/text diagrams stable (independent of insertion order) #5474
Conversation
Amazed that this does not break any tests!!!! |
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 question
@dabacon The PR is ready for a re-review. PTAL! |
cirq-core/cirq/circuits/moment.py
Outdated
@@ -91,6 +91,7 @@ def __init__(self, *contents: 'cirq.OP_TREE') -> None: | |||
ValueError: A qubit appears more than once. | |||
""" | |||
self._operations = tuple(op_tree.flatten_to_ops(contents)) | |||
self._sorted_operations = tuple(sorted(self._operations, key=lambda op: op.qubits)) |
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.
I'm a bit worried about hit we take here for time and memory. Already our circuit code isn't particularly fast and this doubles the memory and time for constructing moments.
Does it make sense to maybe have this be a cached value (like measurement key obs and control keys) that is only populated when used?
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, although I think it would have little value since sorted operations is used in common functions like to_text_diagram
and _eq_
.
…nto origin/fix_moment_str
@dabacon This is ready for another review. PTAL. |
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
Fixes #5404