-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[WIP] Improve speed of sample_counts
from O(N) to O(1)
#8547
Conversation
Use multinomial with parameter N rather than acutally generating N counts.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sampling is now faster. We run the test with more seeds. We still save about 1ms in test time. Furthermore, we increase the number of samples greatly.
Pull Request Test Coverage Report for Build 2864795941
💛 - Coveralls |
For more guidance on how to evaluate this, the introduction of the wikipedia page for multinomial distribution says
Note that distributions over basis states are "categorical distribution"s. This is the same thing I said in #8535. But, here it is stated succinctly with the full authority of Wikipedia(!) |
Another thing: We should check other places in Qiskit (maybe Aer?) where we are effectively sampling from a multinomial distribution. For example, noise modeled by perturbing the circuit randomly with each shot could not be handled as in this PR. |
sample_counts
from $O(N)$ to $O(1)$
sample_counts
from $O(N)$ to $O(1)$sample_counts
from O(N) to O(1)
We've had extensive discussions in the past about it in Aer, in both state vector and MPS simulators. Maybe @chriseclectic or Merav remember the conclusions. This came up also recently with the mock backends of qiskit-experiments (Itamar is the contact point). I'll try to recall myself the discussion and find relevant issues and pull requests. For now I only remember that multinomial was not so magical, but I'll have to recall why. In the case of Aer, part of the story was that Aer is already written in C++, so the question became comparison between different algorithms, without the aspect of using numpy for speeding-up a program. |
I'm sorry that I can't help much more than referring to a search of the word "multinomial" in the Aer repository... https://github.com/Qiskit/qiskit-aer/issues?q=multinomial Similarly, in qiskit-experiments: https://github.com/Qiskit/qiskit-experiments/issues?q=multinomial |
Referring to @yaelbh 's comment above, there was some work in |
Thanks @yaelbh and @merav-aharoni for pointing me to those issues. After reading these, it's clear that this PR needs a bit more work. To be clear, there are two related tasks.
You can perform the counts task by actually generating You can also perform the counts task by generating the counts directly from multinomial distribution, without drawing You can also perform the samples task by first doing the counts task using |
sample_counts
from O(N) to O(1)sample_counts
from O(N) to O(1)
LGTM, but my only concern is it breaks the API, that is it returns different counts even if the seed is the same. |
Thanks for looking at this @ikkoham !
Oh, yes, this needs to be fixed. However, I realized after reading comments from @yaelbh and @merav-aharoni that the method in this PR is in practice not always better. For example, if I have a vector of EDIT: A plot of these experiments is given in #8618 EDIT: I noticed that sampling from multinomial is done in #8137. That could possibly also be done conditionally. Although, I suppose the safest thing is to use multinomial. |
With the move to Rust, this is obsolete. |
Use$O(1)$ method, whereas the current one is $O(N)$ .
numpy.random.multinomial
with parameter N rather than actually generating N counts inQuantumState.sample_counts
. This is anSummary
Use a more efficient algorithm for
QuantumState.sample_counts
.Fixes #8535.
Details and comments
In #8535 a solution using
numpy.random.multinomial
was proposed. This PR implements it.Furthermore, we run the tests for 7 different seeds, rather than just one. The time to run all tests has
decreased from
labels
andcounts_array
are numpy arrays. So it make sense to instead filter the indices and then build new arraysindexing into the two arrays. I latter is slower if the vector of probabilities is not too long. But, it would be probably faster for
large arrays. I suppose optimizing for the larger arrays is best. We could do both with a length cutoff, but that addes
complexity, and I think it's premature optimization at this point.