-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Sampled expectation value for distributions #8748
Conversation
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:
|
Yeah, I put it there as there is similar functionality already. I was also tempted to put in results. Does that work for you? |
I'm fine with results if that's logical to you - I was more just trying to spot the circular import than anything else. |
There is a very funky import chain when loading a circuit because of File ~/Desktop/GitHub/qiskit-terra/qiskit/init.py:27 in So I gave up and moved the imports inside the function call. |
This commit rewrites the python implementation of the expectation value calculation to rust. The rust implementation should be significantly faster to compute the expectation value.
This commit fixes the primary bottleneck in the rust expectation value calculation. The bit string access of the nth position previously used in the code was O(n) and for large numbers of qubits this access could add up in aggregate while computing the expectation value (even with profiling overhead for 127 qubit this was still roughly .3 seconds for calling the rust function). This commit adjusts how we access the nth character of the bit string to be O(1) and removing that bottlneck and for 127 qubit bit string provides a ~3x speedup. The tradeoff is that this isn't safe (in that there is potential incorrect behavior) if someone injects random unicode characters in the bitstring. However as this is unlikely making the change is worth it considering the speed up. After this commit the largest bottleneck is conversion overhead from python->rust and memory management of the temp rust object, at ~59% of the time to run the sampled_expval_complex() function, which is hard to workaround. Although, there might be further tuning to reduce the time required for the remaining 41%.
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.
Some comments below, I think the important one would be the handling of the global coefficient in PauliSumOp
🙂
""" | ||
from .counts import Counts | ||
from qiskit.quantum_info import Pauli, SparsePauliOp | ||
from qiskit.opflow import PauliOp, PauliSumOp |
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.
Should we support PauliOp
? I think all primitives usually only support PauliSumOp
from opflow 😄
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 actually do not know what operators should be used. There are so many different types it is hard for someone not in the weeds of the code to understand which are important and which are not.
If this works then yes indeed. Each operator appears to be coded differently so happy there is some overlap here Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
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'll finish this review. If I have comments on the rust, I'll add them later.
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
LGTM |
Summary
Adds a function
sampled_expectation_value
for computing observables from distribution objects, e.g.Counts
,Quasidistribution
andProbDistribution
. Here, valid observables arestr
,Pauli
,PauliOp
,PauliSumOp
, andSparsePauliOp
. However, unlike built-in operators, this routine allows one to use projectors onto the zero and one states as well using0
and1
in strings, respectively. This allows for computing, for example, Quantum Volume as a expectation value problem.While in general there should be a manner in which to recover the associated variance / standard deviation, I have not done so here because there is as yet no agreed manner in which to report this value.
Details and comments
Here all inputs are converted to strings and coefficients so that a single function can be called. Strings have identity operators removed so that iteration is only over bit values that make a difference in the final output. Ideally input operators would already obey this sparse format for efficiency, but this can be addressed in a follow-on PR.
This is a Python implementation of a routine that is much more suited for a lower-level language like C/C++ and Rust.Thanks to @mtreinish this is now in Rust
Co-authored-by: Matthew Treinish mtreinish@kortar.org