-
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
Fix asv setup and add benchmarks for circuit construction #5845
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.
Big fan of the benchmarking here, and the durations look very manageable for unit tests.
@@ -1,14 +1,14 @@ | |||
{ | |||
// See https://asv.readthedocs.io/en/stable/asv.conf.json.html for documentation. |
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 drop this link? Is ASV sufficiently well-known that readers can be expected to know what this file is without it?
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.
Json files don't usually support comments, so I removed the comment to avoid confusion for anyone reading the json file and wondering why would this work.
@@ -37,8 +37,5 @@ def time_kak_decomposition(target): | |||
cirq.kak_decomposition(target) | |||
|
|||
|
|||
time_kak_decomposition.params = [ # type: ignore | |||
[np.eye(4), SWAP, SWAP * 1j, CZ, CNOT, SWAP.dot(CZ)] | |||
+ [cirq.testing.random_unitary(4) for _ in range(10)] |
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 assume these random unitaries were producing misleading variance in the benchmark?
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.
Main reason to remove is that their string representation (used in benchmarks by default to represent lines) currently shows up as the entire matrix, which clutters the benchmark a lot.
See https://cirq-infra.uc.r.appspot.com/#bench_linalg_decompositions.time_kak_decomposition for example
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.
@95-martin-orion @dstrain115 I've addressed all comments. The PR is ready for a re-review.
@@ -1,14 +1,14 @@ | |||
{ | |||
// See https://asv.readthedocs.io/en/stable/asv.conf.json.html for documentation. |
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.
Json files don't usually support comments, so I removed the comment to avoid confusion for anyone reading the json file and wondering why would this work.
@@ -37,8 +37,5 @@ def time_kak_decomposition(target): | |||
cirq.kak_decomposition(target) | |||
|
|||
|
|||
time_kak_decomposition.params = [ # type: ignore | |||
[np.eye(4), SWAP, SWAP * 1j, CZ, CNOT, SWAP.dot(CZ)] | |||
+ [cirq.testing.random_unitary(4) for _ in range(10)] |
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.
Main reason to remove is that their string representation (used in benchmarks by default to represent lines) currently shows up as the entire matrix, which clutters the benchmark a lot.
See https://cirq-infra.uc.r.appspot.com/#bench_linalg_decompositions.time_kak_decomposition for example
addressed all comments. Merging now.
…#5845) Part of quantumlib#5833 and quantumlib#3840 Fixes quantumlib#4695 Running the benchmarks on master currently gives the following output for circuit construction benchmarks: ```bash [ 75.00%] ··· circuit_construction.NDCircuit.time_circuit_construction ok [ 75.00%] ··· ===================== ============= ============= ============= ============ -- Depth(D) --------------------- ------------------------------------------------------ Number of Qubits(N) 1 10 100 1000 ===================== ============= ============= ============= ============ 1 38.3±0.2μs 235±3μs 2.12±0.01ms 21.2±0.1ms 10 109±0.6μs 901±4μs 8.73±0.02ms 89.2±0.2ms 100 770±4μs 7.32±0.02ms 74.4±0.4ms 764±9ms 1000 7.30±0.02ms 72.0±0.2ms 739±4ms 7.57±0.03s ===================== ============= ============= ============= ============ [ 87.50%] ··· circuit_construction.SurfaceCode_Rotated_Memory_Z.time_surface_code_circuit_construction ok [ 87.50%] ··· ========== ============= distance ---------- ------------- 3 1.97±0.01ms 5 7.67±0.07ms 7 22.6±0.1ms 9 59.8±0.4ms 11 122±0.7ms 13 232±1ms 15 407±2ms 17 701±2ms 19 1.09±0.02s 21 1.60±0.02s 23 2.25±0.01s 25 3.24±0.06s ========== ============= [100.00%] ··· circuit_construction.SurfaceCode_Rotated_Memory_Z.track_surface_code_circuit_operation_count ok [100.00%] ··· ========== ========= distance ---------- --------- 3 369 5 3225 7 12985 9 36369 11 82401 13 162409 15 290025 17 481185 19 754129 21 1129401 23 1629849 25 2280625 ========== ========= ``` Note that the circuit construction for d21 surface code takes only 1.6 seconds. This is because we are constructing moment-by-moment instead of appending operations (in which case the earliest strategy takes a significant amount of time). Going forward, we should add more benchmarks for different use cases. Once this is merged, https://cirq-infra.uc.r.appspot.com/ would be updated with the new benchmarks and we can see the improvement / regressions we make to circuit construction performance over time / commits. cc @dabacon @maffoo @dstrain115
Part of #5833 and #3840
Fixes #4695
Running the benchmarks on master currently gives the following output for circuit construction benchmarks:
Note that the circuit construction for d21 surface code takes only 1.6 seconds. This is because we are constructing moment-by-moment instead of appending operations (in which case the earliest strategy takes a significant amount of time).
Going forward, we should add more benchmarks for different use cases. Once this is merged, https://cirq-infra.uc.r.appspot.com/ would be updated with the new benchmarks and we can see the improvement / regressions we make to circuit construction performance over time / commits.
cc @dabacon @maffoo @dstrain115