-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add ASV benchmarks for BasicSimulator Clifford optimization #15448
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
base: main
Are you sure you want to change the base?
Add ASV benchmarks for BasicSimulator Clifford optimization #15448
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 20265672539Details
💛 - Coveralls |
alexanderivrii
left a comment
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.
Thanks for adding these well thought-out set of benchmarks @debasmita2102. A few minor comments, but otherwise LGTM.
| def setup(self, n_qubits): | ||
| """Setup random Clifford circuit for given n_qubits.""" | ||
|
|
||
| cliff = random_clifford(n_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.
This call to random_clifford is not deterministic, consider providing an explicit seed, for example:
| cliff = random_clifford(n_qubits) | |
| cliff = random_clifford(n_qubits, seed=0) |
I now see that there are other ASV tests that use random_clifford with non-deterministic results, I think we also fix those (but in a separate PR).
| def time_statevector(self, n_qubits): | ||
| """Time statevector simulation of random Clifford circuit.""" | ||
| _ = n_qubits # ASV parameter |
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.
Would something like this work as well?
| def time_statevector(self, n_qubits): | |
| """Time statevector simulation of random Clifford circuit.""" | |
| _ = n_qubits # ASV parameter | |
| def time_statevector(self, _n_qubits): | |
| """Time statevector simulation of random Clifford circuit.""" |
| def time_clifford(self, n_qubits): | ||
| """Time Clifford-optimized simulation of random Clifford circuit.""" | ||
| _ = n_qubits # ASV parameter |
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.
Same comment as before.
| """Time Clifford-optimized simulation of random Clifford circuit.""" | ||
| _ = n_qubits # ASV parameter | ||
| backend = BasicSimulator() | ||
| backend.run( |
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.
Would it make sense to add _ = here for readability, expressing that the call returns a result (which is however ignored)?
| backend.run( | |
| _ = backend.run( |
If you think it does, then this comment applies to other similar calls in this file.
| from qiskit.providers.basic_provider import BasicSimulator | ||
| from qiskit.quantum_info import random_clifford | ||
|
|
||
| # pylint: disable=attribute-defined-outside-init |
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 don't know if it matters (and most probably it does not), but in all other tests the # pylint: disable line appears before the module dostring/imports. Feel free to change this or leave it as is.
| def time_statevector(self, n_qubits): | ||
| """Time statevector simulation of GHZ circuit.""" | ||
| _ = n_qubits # ASV parameter | ||
| backend = BasicSimulator() |
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.
This is fast, right (otherwise, you could also move this construction to setup as well).
|
Note that we are able merge this PR before #15159: calling |
ShellyGarion
left a comment
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.
@debasmita2102 - Thanks for adding ASV tests!
Are there similar tests for the BasicSimulator for non-Clifford circuits? If not, maybe it's worth adding them in a seperate PR.
My main comment is that with a Clifford simulator we can extend the number of qubits in the tests for more than 24. But this can be done only after we merge #15159.
| class BasicSimulatorGHZBenchmark: | ||
| """Benchmark BasicSimulator on GHZ Clifford circuits.""" | ||
|
|
||
| params = ([4, 8, 12, 16],) |
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 wonder if after we merge #15159, we could extend the number of qubits here
| class BasicSimulatorRandomCliffordBenchmark: | ||
| """Benchmark BasicSimulator on random Clifford circuits.""" | ||
|
|
||
| params = ([4, 8, 12, 16],) |
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 wonder if after we merge #15159, we could extend the number of qubits here
Summary
Add ASV benchmarks to track performance of the
BasicSimulatorClifford (StabilizerState) path versus the existing statevector path.The benchmarks are split out from PR Add Clifford circuit optimization to BasicSimulator #15159 so that code changes and performance tracking can be reviewed independently.
Details and comments
BasicSimulatorGHZBenchmarkto compare Clifford vs statevector performance on GHZ-like Clifford circuits (two output bitstrings) forn_qubits = [4, 8, 12, 16].BasicSimulatorRandomCliffordBenchmarkto compare Clifford vs statevector performance on random Clifford circuits generated viarandom_clifford(n_qubits)for the same qubit sizes.