-
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
Dedicated method for creating circuit from op tree with EARLIEST strategy #5332
Conversation
This reverts commit 9c1d3ed.
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.
Nice speedup! Looking forward to having this in. Was thinking of looking into this the other day and am now very happy that it is already taken care of!
I added some notes that this should have more commenting.
cirq-core/cirq/circuits/circuit.py
Outdated
else: | ||
self.append(contents, strategy=strategy) | ||
|
||
def _create_from_earliest(self, contents): |
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.
We may want to consider adding a docstring or other comments, especially since there are subtleties here in how to do earliest (with measurement keys, controlled ops, etc).
I certainly find it challenging to follow without comments.
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.
Added comments and types. LMK what you think.
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.
Very nice!
@dstrain115 I updated the documentation. Should this be merged before 0.15? |
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.
Sorry, I lost track of this. Let's merge this!
Merge label? I don't have authz |
# limit index to 0..len(self._moments), also deal with indices smaller 0 | ||
k = max(min(index if index >= 0 else len(self._moments) + index, len(self._moments)), 0) | ||
for moment_or_op in moments_and_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.
Link
…tegy (quantumlib#5332) We keep track of the latest moment that contains each qubit and measurement key. Then we know where to put each operation in constant time. We don't create the actual Moments until the very end, when we know where everything goes. Also added explicit key protocol impls for EigenGate, preempting the protocol from attempting a bunch of fallback options. On my laptop this speeds up creating circuits with EARLIEST strategy by almost infinity percent. (On my laptop, the circuit in the new test goes from 29.00s on master to 0.13s here).
…tegy (quantumlib#5332) We keep track of the latest moment that contains each qubit and measurement key. Then we know where to put each operation in constant time. We don't create the actual Moments until the very end, when we know where everything goes. Also added explicit key protocol impls for EigenGate, preempting the protocol from attempting a bunch of fallback options. On my laptop this speeds up creating circuits with EARLIEST strategy by almost infinity percent. (On my laptop, the circuit in the new test goes from 29.00s on master to 0.13s here).
We keep track of the latest moment that contains each qubit and measurement key. Then we know where to put each operation in constant time. We don't create the actual Moments until the very end, when we know where everything goes. Also added explicit key protocol impls for EigenGate, preempting the protocol from attempting a bunch of fallback options. On my laptop this speeds up creating circuits with EARLIEST strategy by almost infinity percent. (On my laptop, the circuit in the new test goes from 29.00s on master to 0.13s here).