Skip to content
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

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

jakelishman
Copy link
Member

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 the QuantumVolume builder to use more efficient circuit construction methods.

Details and comments

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.
@jakelishman jakelishman added performance Changelog: New Feature Include in the "Added" section of the changelog Changelog: API Change Include in the "Changed" section of the changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Mar 28, 2024
@jakelishman jakelishman added this to the 1.1.0 milestone Mar 28, 2024
@jakelishman jakelishman requested a review from a team as a code owner March 28, 2024 15:58
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

@coveralls
Copy link

coveralls commented Mar 28, 2024

Pull Request Test Coverage Report for Build 8589314293

Details

  • 26 of 26 (100.0%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.009%) to 89.393%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 3 93.13%
Totals Coverage Status
Change from base Build 8577020881: 0.009%
Covered Lines: 60307
Relevant Lines: 67463

💛 - Coveralls

@levbishop
Copy link
Member

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.

@jakelishman
Copy link
Member Author

jakelishman commented Mar 29, 2024

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 su4_<seed>", I guess I don't fully understand the benefit: you don't need a seed in those cases, because you've got the full matrix right to hand - it's in the gate parameters.

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 qv = QuantumVolume(..., flatten=True); qv.data[<index>].operation to get the underlying unitary gate.

@levbishop
Copy link
Member

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))
Copy link
Member

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).

Copy link
Member Author

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.

@jakelishman jakelishman added this pull request to the merge queue Apr 8, 2024
Merged via the queue into Qiskit:main with commit c99f325 Apr 8, 2024
12 checks passed
@jakelishman jakelishman deleted the fast-qv branch April 8, 2024 21:37
github-merge-queue bot pushed a commit to qiskit-community/qiskit-experiments that referenced this pull request Apr 22, 2024
### Summary

This PR updates the stored probabilities in
`test_qv_ideal_probabilities` after the randomization update in
Qiskit/qiskit#12097.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog Changelog: New Feature Include in the "Added" section of the changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants