-
Notifications
You must be signed in to change notification settings - Fork 746
randomize benchmarking performace benchmark #306
Conversation
Before this can merge we'll need to make sure we're installing qiskit-ignis as part of the asv venv creation. I'll push up the modification to do that |
# number of Cliffords in the sequence (start, stop, steps) | ||
length_vector = np.arange(1, 200, 4) | ||
|
||
params = (nq, nseeds, length_vector) |
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.
If we're not going to vary the number of qubits or the number of seeds I don't think we need to make them parameters. Was the intent to run with a different number of qubits and seeds at some point in the future?
Also this is in the wrong format. If you're using multiple params like this it's supposed to be a tuple of iterators (or lists, arrays, or other things that support iter) and then asv will build out the matrix of benchmarks to run using the combinations of all the parameters from the elements in each parameter iterator. For example you could do something like:
params = ([2,3,4,5], [20,30,40], [np.arange(1, 200, 4), np.arange(1,400, 8)])
param_names = ['nq', 'nseeds', 'length_vector']
If everything is going to be static you don't need to use params.
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.
|
||
self.sim_backend = QasmSimulatorPy() | ||
|
||
def time_simulator_transpile(self, _, __): |
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.
Also forgot to mention if you're setting 3 parameters here you'll need to add a 3rd parameter to the benchmark functions, because each parameter gets passed to setup()
and each benchmark method. Since you only need to use them in setup() they're just blank here.
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.
Hi is there any update on this? |
I will continue to work on it after I finish my work on purity rb in ignis |
6dfc115
to
cd60310
Compare
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 definitely getting closer, but I'm not able to run these benchmarks locally. When I attempt to by running asv dev --bench randomized
everything fails with an error like:
For parameters: 3, 10, [[0, 2], [1]], [1, 3], array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121,
131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251,
261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381,
391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491])
Traceback (most recent call last):
File "/usr/lib/python3.7/site-packages/asv/benchmark.py", line 1039, in main_run_server
main_run(run_args)
File "/usr/lib/python3.7/site-packages/asv/benchmark.py", line 907, in main_run
skip = benchmark.do_setup()
File "/usr/lib/python3.7/site-packages/asv/benchmark.py", line 458, in do_setup
result = Benchmark.do_setup(self)
File "/usr/lib/python3.7/site-packages/asv/benchmark.py", line 390, in do_setup
setup(*self._current_params)
File "/home/mtreinish/git/qiskit/qiskit/test/benchmarks/randomized_benchmarking.py", line 76, in setup
seed=random_seed)
File "/home/mtreinish/git/qiskit/qiskit/test/benchmarks/randomized_benchmarking.py", line 50, in build_rb_circuit
rb_circs, _ = rb.randomized_benchmarking_seq(**rb_opts)
File "/usr/lib/python3.7/site-packages/qiskit/ignis/verification/randomized_benchmarking/circuits.py", line 183, in randomized_benchmarking_seq
qlist_flat, n_q_max = check_pattern(rb_pattern)
File "/usr/lib/python3.7/site-packages/qiskit/ignis/verification/randomized_benchmarking/circuits.py", line 75, in check_pattern
pattern_flat.extend(pat)
TypeError: 'int' object is not iterable
except OSError: | ||
skip_msg = ('Skipping tests for %s qubits because ' | ||
'tables are missing' % str(nq)) | ||
print(skip_msg) |
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.
In the case of an OSError we'll print this message to stdout and raise a NameError marking that benchmark as failed. Instead we should raise a NotImplementedError
here to mark the benchmark as skipped (see: https://asv.readthedocs.io/en/latest/writing_benchmarks.html#setup-and-teardown-functions ):
print(skip_msg) | |
raise NotImplementedError(skip_msg) |
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.
dcdd646
to
c53a70c
Compare
Indeed - the length_multiplier depended on the specific rb_pattern, so I removed this option. |
I just noticed your force push removed my commit (dcdd646 ) that added ignis to the asv conf. If you need to do another force push just make sure you include that commit in your local branch. |
I'm still getting local failures with this (basically the same one, just with a numpy int type instead of python's int):
|
226975d
to
7b3ca9f
Compare
I do not know why it's not working, as this option of parameters works well for me:
Several comments:
namely, remove the option [[0, 2], [1]] for 1&2 qubits simultaneously. |
@ShellyGarion it was using the latest release as per what I added in: Qiskit/qiskit@b03f674 I did that because these benchmarks track terra so we want to minimize the differences between consecutive runs. If we track master that will potentially change things every time we merge a change to ignis (skewing the results). Is qiskit-community/qiskit-ignis#174 necessary for this benchmark to work? As for being near the last option combination, that's just because I was lazy and didn't scroll up to the top, it wasn't a ram issue. Also every single parameter combination failed. |
I did some local debugging and pushed: Qiskit/qiskit@bb28c79 which fixes the issue I was hitting locally. We were previously passing the length vector as the RB pattern (and vice versa), that fixes the order. |
Thanks @mtreinish ! Is the code ready to be merged now? |
No, not quite yet; it's still failing. This time it looks like because the function to build the rb circuits is returning more than just circuits and the transpile calls fail because we're passing it not a circuit. I've got a fix pending to address this issue as well. |
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.
Ok, I've pushed the last fix which makes it all run successfully locally: Qiskit/qiskit@b79159e
[ 75.00%] ··· ======== =============== ======================================================================================================================================================================================================================================================================================== ============
nseeds rb_pattern length_vector
-------- --------------- ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- ------------
1 [[0]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 312±4ms
1 [[0]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 678±4ms
1 [[0, 1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 1.00±0s
1 [[0, 1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 2.31±0s
1 [[0, 2], [1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 1.26±0.01s
1 [[0, 2], [1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 3.09±0.05s
5 [[0]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 303±6ms
5 [[0]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 711±8ms
5 [[0, 1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 1.01±0s
5 [[0, 1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 2.37±0.02s
5 [[0, 2], [1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 1.30±0s
5 [[0, 2], [1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 3.05±0.01s
10 [[0]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 313±2ms
10 [[0]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 676±4ms
10 [[0, 1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 1.01±0s
10 [[0, 1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 2.39±0.01s
10 [[0, 2], [1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 1.27±0s
10 [[0, 2], [1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 3.14±0s
======== =============== ======================================================================================================================================================================================================================================================================================== ============
[100.00%] ··· ...nchmarkingBenchmark.time_simulator_transpile ok
[100.00%] ··· ======== =============== ======================================================================================================================================================================================================================================================================================== ============
nseeds rb_pattern length_vector
-------- --------------- ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- ------------
1 [[0]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 197±10ms
1 [[0]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 441±20ms
1 [[0, 1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 550±20ms
1 [[0, 1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 1.31±0.02s
1 [[0, 2], [1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 736±20ms
1 [[0, 2], [1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 1.73±0.02s
5 [[0]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 210±8ms
5 [[0]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 462±8ms
5 [[0, 1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 554±6ms
5 [[0, 1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 1.34±0.02s
5 [[0, 2], [1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 739±8ms
5 [[0, 2], [1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 1.71±0.01s
10 [[0]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 224±7ms
10 [[0]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 479±3ms
10 [[0, 1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 578±4ms
10 [[0, 1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 1.36±0.01s
10 [[0, 2], [1]] array([ 1, 5, 9, 13, 17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97, 101, 105, 109, 113, 117, 121, 125, 129, 133, 137, 141, 145, 149, 153, 157, 161, 165, 169, 173, 177, 181, 185, 189, 193, 197]) 741±0.7ms
10 [[0, 2], [1]] array([ 1, 11, 21, 31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181, 191, 201, 211, 221, 231, 241, 251, 261, 271, 281, 291, 301, 311, 321, 331, 341, 351, 361, 371, 381, 391, 401, 411, 421, 431, 441, 451, 461, 471, 481, 491]) 1.67±0s
======== =============== ======================================================================================================================================================================================================================================================================================== ============
However, it still can't merge yet it's way to slow to add to the benchmark suite. Running the full battery of benchmarks locally took 1.5 hours on a single terra commit (which is about 3x the time of the rest of the benchmark suite) to complete both benchmarks with the full 24 combination of input parameters. (it runs each benchmark multiple times) This is a non-starter since as we run the benchmarks over each merged commit (and my local desktop is faster than the baremetal machine we have provisioned for dedicated benchmarking). I think we need to do 2 things here first is switch the rb circuit build from the setup
method to setup_cache
, see: https://asv.readthedocs.io/en/stable/writing_benchmarks.html#setup-and-teardown-functions the second thing is remove all the parametrization from the benchmark and just use a single parameter. Both of these should hopefully mitigate the run time issue and enable us to run this on every commit.
Thanks @mtreinish !
Indeed, we see in the results that the parameter |
b08d3c6
to
fabd653
Compare
@mtreinish - I changed the parameters, so now we only have one option for |
@ShellyGarion you can easily run it locally too. You just need to have asv installed That being said I ran it locally and it only took about 25mins to run. While that is a significant reduction I still feel it is too slow. A normal full benchmark run without these new benchmarks takes about 30-35min on my desktop so we'll be bumping the run time by a lot if we add that much time on top of it. I also looked more into This also raises a question for me about where we're actually spending all the time here. When you look at the benchmark results:
each transpile call is actually not that slow, taking in the worst case ~3 seconds even factoring in that asv runs each test multiple times it doesn't add up to 25mins (I did also just find a bug that I'll push a patch up for shortly which increases the number of circuits we transpile). It feels like we're spending most of the time building the circuits. But when I run it locally it's also relatively quick so I'm not sure what is taking so long |
This commit fixes a number of issues with the benchmark. First it removes single entry parameters from the parameter list, there's no reason to make them parameters if we don't use multiple values. It also fixes a small bug in the return from the build_rb_circuit() function where we were only returning one sequence of circuits instead of the whole set. It also adjusts how we use random values by properly setting a seed for both circuit building and tranpile. The last thing it does is for consistency it pins the ignis version to a single version and sets the benchmark version number to match that. This way if/when we need to bump the ignis version we use in the benchmarks we'll have to bump the benchmark's version too to match. This way we can make sure we're using the same ignis version for all benchmark results.
I just pushed up: Qiskit/qiskit@31380d5 which fixes several issues I identified in the benchmarks. But, we still will want to reduce it to be a single length vector for time. Unless we can figure out what is causing it to take so long. Maybe we also want to time building the rb circuits not just the time to transpile them. |
This is strange. From my experience, usually generating the circuits is much faster than transpiling & running them. Perhaps you can only try the option: |
class RandomizedBenchmarkingBenchmark: | ||
# parameters for RB (1&2 qubits): | ||
params = ([[[0]], [[0, 1]], [[0, 2], [1]]], | ||
[np.arange(1, 200, 4), np.arange(1, 500, 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.
Instead of using the raw arrays as parameters (giving in the long lines in the asv output), maybe use a tuple of (start, stop, step)
as a parameter and build the length vector in setup
?
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 not sure, since sometimes we may like to use other sequences (say lengths that grow exponentially and not linearly)
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.
Hmm, maybe then a tuple of (sequence_type, sequence_args)
? The parameters will end up being displayed in the web interface (see e.g. https://qiskit.github.io/qiskit/#circuit_construction.CircuitConstructionBench.time_circuit_extend ) and the full list would be quite unwieldy.
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 last commit handles this, since there is only a single length_vector
, so this parameter is not needed anymore.
Yeah I think I have also seen most of the time spent in transpile, specifically the Unroller pass. @chriseclectic saw this recently too I think? |
It's entirely possible the cost of circuit construction has increased recently, terra has had some regressions in its circuit building performance recently. I tested a quick benchmark that only builds the circuits and it was comparable in time to the transpilation benchmarks.
|
There definitely has been a large regression in the performance of circuit construction introduced by Qiskit/qiskit#2414: https://qiskit.github.io/qiskit/#circuit_construction.CircuitConstructionBench.time_circuit_extend?p-width=20&p-gates=2048&commits=79c44e56 |
Is this PR ready to be merged? |
Can you reduce it to a single vector (presumably |
This commit removes one of the two length vectors we were using for constructing the RB circuits. This is done in the interest of time since the RB benchmarking can be quite slow so this reduces the number of input parameters we iterate over which decreases the total run time of RB benchmarks. If in the future we improve the performance so that the impact is minimal to the full asv run we can add back additional length vectors for benchmarking.
Thanks @mtreinish ! Is this PR ready to be merged now? |
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 LGTM now I think (after I updated the license header). I have one inline comment, which is more about how much data we want to backfill after we merge this.
|
||
import numpy as np | ||
import qiskit.ignis.verification.randomized_benchmarking as rb | ||
from qiskit.providers.basicaer import QasmSimulatorPy |
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 only thing left that I see here is this line. Depending on how far back we want to backfill the benchmark data this will be the limiting factor since it was added in Qiskit/qiskit#1854 which is post 0.7.0. If we wanted data going back to 0.7 we'll have to handle that here.
@mtreinish is there a reason you did not merge after approving? |
@jaygambetta not really, there was an open question on about the length of history to build, but it's not a blocker. I just didn't merge before because I was just waiting on CI to finish and then forgot to come back and hit the merge button. |
Thanks @mtreinish ! |
…e#306) * first version of benchmark/randomize_benchmarking.py * fix lint error * fixed a misprint * updated benchmark test for RB * fixed RB benchmark test following comment * fix lint error * Add qiskit-ignis to benchmark env * removed nq from the parameters * Correct setup() parameter order * Only return circuits for transpilation * nseeds = [1] * Fix various issues with the benchmark This commit fixes a number of issues with the benchmark. First it removes single entry parameters from the parameter list, there's no reason to make them parameters if we don't use multiple values. It also fixes a small bug in the return from the build_rb_circuit() function where we were only returning one sequence of circuits instead of the whole set. It also adjusts how we use random values by properly setting a seed for both circuit building and tranpile. The last thing it does is for consistency it pins the ignis version to a single version and sets the benchmark version number to match that. This way if/when we need to bump the ignis version we use in the benchmarks we'll have to bump the benchmark's version too to match. This way we can make sure we're using the same ignis version for all benchmark results. * Reduce the number of length vectors used to 1 This commit removes one of the two length vectors we were using for constructing the RB circuits. This is done in the interest of time since the RB benchmarking can be quite slow so this reduces the number of input parameters we iterate over which decreases the total run time of RB benchmarks. If in the future we improve the performance so that the impact is minimal to the full asv run we can add back additional length vectors for benchmarking. * Update copyright header to use consistent format
…e#306) * first version of benchmark/randomize_benchmarking.py * fix lint error * fixed a misprint * updated benchmark test for RB * fixed RB benchmark test following comment * fix lint error * Add qiskit-ignis to benchmark env * removed nq from the parameters * Correct setup() parameter order * Only return circuits for transpilation * nseeds = [1] * Fix various issues with the benchmark This commit fixes a number of issues with the benchmark. First it removes single entry parameters from the parameter list, there's no reason to make them parameters if we don't use multiple values. It also fixes a small bug in the return from the build_rb_circuit() function where we were only returning one sequence of circuits instead of the whole set. It also adjusts how we use random values by properly setting a seed for both circuit building and tranpile. The last thing it does is for consistency it pins the ignis version to a single version and sets the benchmark version number to match that. This way if/when we need to bump the ignis version we use in the benchmarks we'll have to bump the benchmark's version too to match. This way we can make sure we're using the same ignis version for all benchmark results. * Reduce the number of length vectors used to 1 This commit removes one of the two length vectors we were using for constructing the RB circuits. This is done in the interest of time since the RB benchmarking can be quite slow so this reduces the number of input parameters we iterate over which decreases the total run time of RB benchmarks. If in the future we improve the performance so that the impact is minimal to the full asv run we can add back additional length vectors for benchmarking. * Update copyright header to use consistent format
Summary
#272
Details and comments