-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Improve performance and randomness of QuantumVolume
#12097
Conversation
This hugely improves the construction time of `QuantumVolume` circuits, in part by removing the previous behaviour of using low-entropy indiviual RNG instances for each SU4 matrix. Now that we need larger circuits, this would already have been a non-trivial biasing of the random outputs, but also, calling Scipy random variates in a loop is _much_ less efficient than vectorising the entire process in one go. Along with changes to the RNG, this commit also adds a faster path to `UnitaryGate` to shave off some useless repeated calculations (this can likely be used elsewhere in Qiskit) and reworks the `QuantumVolume` builder to use more efficient circuit construction methods.
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 8589314293Details
💛 - Coveralls |
The perf improvements are desirable but as I recall the slowness and bias was understood and still chosen for the original implementation due to some requirement of reproducibility. Perhaps @ajavadia remembers the details. If we no longer require that type of reproducibility this seems otherwise fine. |
I'm not sure I understand where the reproducibility problems arise, here. The class is still 100% reproducible if you set a seed. All that you lose here is that if you don't set a seed, we don't hamstring the randomness in order to provide a seed that can be used to reproduce the gate individually. With the "output gates are named I think the relative merits of the biasing have potentially changed now as well, since we're running much much larger circuits than we were at the time the class was added. Drawing the matrices from a list of 1000 isn't such a big deal when your circuit only has a few tens of them, but if we're starting to go the "it's not science unless you're using 5k gates" route, then limiting to 1000 SU4 matrices effectively guarantees duplication and heavy biasing. fwiw, the discussion of the names seems to be in this thread: #4867 (comment). If the intention of labelling gates is so that you can point at a gate in the drawing and more easily jump to its definition (I'm not sure why exactly you'd want this, though), we could label them with integer offsets so you can do |
I think perhaps the reproducibility was for export/import of QV circuits to test on non-qiskit stacks? Nevertheless I think thats better handled by making qasm or qpy export is properly transparent rather than adding this bias to QV circuts. |
@@ -97,7 +100,7 @@ def __init__( | |||
# Convert to numpy array in case not already an array | |||
data = numpy.asarray(data, dtype=complex) | |||
input_dim, output_dim = data.shape | |||
num_qubits = int(math.log2(input_dim)) | |||
num_qubits = num_qubits if num_qubits is not None else int(math.log2(input_dim)) |
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 num_qubits
optimization seems a bit unecessary (30ns vs 120ns on my machine, but fine).
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.
That's fair - I did this when it was still np.log2
and it was a huge overhead, and I didn't really think to switch it from the Numpy overhead at the time.
### Summary This PR updates the stored probabilities in `test_qv_ideal_probabilities` after the randomization update in Qiskit/qiskit#12097.
Summary
This hugely improves the construction time of
QuantumVolume
circuits, in part by removing the previous behaviour of using low-entropy indiviual RNG instances for each SU4 matrix. Now that we need larger circuits, this would already have been a non-trivial biasing of the random outputs, but also, calling Scipy random variates in a loop is much less efficient than vectorising the entire process in one go.Along with changes to the RNG, this commit also adds a faster path to
UnitaryGate
to shave off some useless repeated calculations (this can likely be used elsewhere in Qiskit) and reworks theQuantumVolume
builder to use more efficient circuit construction methods.Details and comments