Speed up random_circuit
#8983
Speed up random_circuit
#8983
Conversation
There is no need to use the slower `append` (which adds a lot of checking overhead for large circuits) since we fully control the construction of the circuit. This also overhauls all the randomisation components to produce all the necessary randomness for a given layer in a single go. This massively reduces the Python-space and Numpy-dispatch overhead of the nested loop. The changes to the randomisation in this new form mean that on average, more gates will be generated per circuit, because the output will choose 1q gates more frequently than it chooses 2q, which in turn will be more frequent than 3q, but each layer of the "depth" still has to involve every qubit. The change is because the likelihood of choosing a gate with a given number of qubits is now proportional to how many different gates of that number of qubits there are in the options. For sample timings, the call `random_circuit(433, 1000, seed=1)` went from 15.2s on my machine (macOS Python 3.10, Intel i7 @ 2.3 GHz) down to 2.3s, so a speed-up of about 6-7x.
jakelishman
added
Changelog: New Feature
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 3345611854
💛 - Coveralls |
There was a problem hiding this comment.
This is a really cool PR, pre-computing the contents each layer with array ops in numpy up front to limit the overhead and rng interaction is a really good idea (and something I wouldn't have thought of). I definitely had to refer to the numpy docs a couple of times and learned about some new functions like searchsorted.
Nothing major really stood out to me while reviewing this, my only thought is since this effectively is going to fix #6994 we should probably add an explicit test for it.
| if max_operands >= 3: | ||
| gates.extend(gates_3q) | ||
| gates = np.array( | ||
| gates, dtype=[("class", object), ("num_qubits", np.int64), ("num_params", np.int64)] |
There was a problem hiding this comment.
I think this is the first time I've ever seen field dtypes used in practice before. I had completely forgotten you could even do this in numpy. This is a cool application for it.
There was a problem hiding this comment.
Yeah, I only very rarely use them. For posterity: the reasons for the choice here is that I wanted the most efficient access from rng.choice (which also maintains dtype in its output), and for the subsequent cumulative sums over num_qubits and num_params to have defined strides in their access patterns, so the Numpy vectorisation after the rng choice would all work as expected.
|
Depending on how we feel about this chain: #8983 (comment), this is ready for re-review. |
This deliberately does not use the `get_standard_gate_name_mapping` function, in order to avoid changes to that function breaking RNG compatibility in this unrelated function.
There was a problem hiding this comment.
Approach LGTM, a few small comments. Also, looks like the random circuit generation uncovered probably a missing rule in the equivalence library: https://dev.azure.com/qiskit-ci/qiskit-terra/_build/results?buildId=40983&view=logs&j=e4ea27c2-3a16-5b32-7be8-0bf30fe9e828&t=793d4f07-8490-552a-798f-6a5657042d16&l=17488 .
| The exact circuit returned by ``qiskit.circuit.random.random_circuit`` for a | ||
| given seed has changed. This is due to efficiency improvements in the | ||
| internal random-number generation for the function. |
There was a problem hiding this comment.
Does this need a release note? In general, I don't think we hold any of our functionality to seed-reproducible output across different Terra verions (or numpy versions, platforms, etc.). It would be good to have canonical documentation one way or the other though.
There was a problem hiding this comment.
We have done in the past - we certainly did it for the switch of SabreSwap to Rust (and my intent was to write another of those notes when I break the RNG compat in my new version of Sabre too).
I'm approximately in favour of mentioning it in the release notes - we use seed stability in our tests (e.g. the SabreLayout tests), so it's not inconceivable that others are doing similar things.
There was a problem hiding this comment.
Yeah, I would agree we're not committed to ensuring seed stability across releases, but it's still good to document it as an upgrade note, especially for functions like this (and sabre) where the output is used for testing. Just to document it'll be different for users when they upgrade from the previous release to the new one. We have done release notes like this in the past for random_circuit too: https://qiskit.org/documentation/release_notes.html#release-notes-0-19-0-upgrade-notes (it's in there a bit down the list).
|
I've added the missing equivalences in #9017, which this PR now depends on for its tests to pass. |
|
#9017 is merged, so hopefully these tests should pass now. |
Summary
There is no need to use the slower
append(which adds a lot of checking overhead for large circuits) since we fully control the construction of the circuit.This also overhauls all the randomisation components to produce all the necessary randomness for a given layer in a single go. This massively reduces the Python-space and Numpy-dispatch overhead of the nested loop.
The changes to the randomisation in this new form mean that on average, more gates will be generated per circuit, because the output will choose 1q gates more frequently than it chooses 2q, which in turn will be more frequent than 3q, but each layer of the "depth" still has to involve every qubit. The change is because the likelihood of choosing a gate with a given number of qubits is now proportional to how many different gates of that number of qubits there are in the options.
For sample timings, the call
random_circuit(433, 1000, seed=1)went from 15.2s on my machine (macOS Python 3.10, Intel i7 @ 2.3 GHz) down to 2.3s, so a speed-up of about 6-7x.Details and comments
If we want to make the circuit generate as many 3q gates as it did before, it's pretty trivial to adjust the weights in the
rng.choicecall to restore the same sort of behaviour as before.cc: @kdk, @ismaelfaro. Ismael: I'm tagging you just because it might be of interest, since I know you were using this function and finding it slow before.
Fix #6994.
Close #8425. (obsoleting a stale PR)