-
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
Add performance benchmarks for CQC circuit routing #5869
Conversation
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 w/ remarks
benchmarks/transformers/routing.py
Outdated
class RouteCQC: | ||
params = [[10, 25, 50, 75, 100], [10, 50, 100, 250, 500, 1000], [0.5], [10]] | ||
param_names = ["qubits", "depth", "op_density", "grid_device_size"] | ||
timeout = 120 # Increase timeout to 2 minutes instead of default 60 seconds. |
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.
The 100 / 1000 / 0.5 / 10 case is failing because of timeout. I ran it locally and it takes 258s (well above 2mins). So this test would have to be removed or the timeout should be increased to ~5 mins. However, maybe this can be ignored for now since the optimization would bring it down to ~90s.
def setup(self, qubits: int, depth: int, op_density: float, grid_device_size: int): | ||
gate_domain = {cirq.CNOT: 2, cirq.X: 1} | ||
self.circuit = cirq.testing.random_circuit( | ||
qubits, depth, op_density, gate_domain=gate_domain, random_state=12345 |
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.
For smaller circuits, should this be run on multiple seeds and averaged?
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 could create a separate benchmark for it if we see value in changing seeds. I think this benchmark provides a decent approximation of scaling with number of qubits and circuit depth and the numbers look pretty consistent to me.
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.
The table has "failed" values for the largest circuit, please decrease the maximum size.
The duration of asv run
on my desktop is 45 minutes.
Do we have some limit for the maximum duration of the benchmarks run?
I've increased the timeout to 3 minutes for now. We can also have failed values for some combinations of a parameterized benchmark and the stored results and plots work as expected (no data points appear for a failed benchmark)
This is expected for now since asv doesn't allow running benchmarks in parallel. Also, benchmarks are supposed to be executed on a cloud machine asynchronously and render plots of the form: https://cirq-infra.uc.r.appspot.com/ |
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 please update the initial comment with a valid timing.
* Add benchmarks for circuit routing * Add more data points * Increase timeout to 5 minutes
Part of #3840
Would be useful to track the improvements in runtime after #5860 is fixed.
Testing via
asv run --quick
locally gives the following output:cc @ammareltigani