Add reference implementation of EstimatorV2 #11227
Conversation
ikkoham
added
Changelog: New Feature
|
|
||
| def __init__( | ||
| self, | ||
| observables: Union[BasisObservableLike, ArrayLike], |
There was a problem hiding this comment.
There was this comment in pec-runtime's ObservablesArray:
# NOTE: With support for arrays of observables, its ambiguous how
# to treat a non-sequence input, so we should deprecate passing in
# a single BasisObservableLike
I'm not sure why single BasisObservableLike is ambiguous, but it'd need to be converted or tolist() won't return a list.
| self.parameter_values.validate() | ||
| # Cross validate circuits and observables | ||
| for i, observable in enumerate(self.observables): | ||
| num_qubits = len(next(iter(observable))) |
There was a problem hiding this comment.
This validation doesn't work when passing in a PauliList for a circuit.
There was a problem hiding this comment.
PauliList will be deprecated #11055
Pull Request Test Coverage Report for Build 7707314260Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
|
Will you implement |
|
How about moving bindings_array.py, ovservables_array, and shape.py to a new directory |
Sure. It's in the TODO list in the issue comment. I am waiting for internal discussions for DataBin. |
|
Do you plan to deprecate public methods in qiskit.primitives.utils? I think they are not used when qiskit.algorithms and PrimtiivesV1 are removed. |
IMO, at the same time with PrimitiveV1, so 0.46? |
ikkoham
force-pushed
the
primitives/estimator-v2
branch
from
4cd5838 to
0e428a9
Compare
|
One or more of the the following people are requested to review this:
|
There was a problem hiding this comment.
The raises on precision > 0 needs to be removed. Other than that I would suggest adding more detailed API documentation to this class rather than referring to BaseEstimatorV2's documentation since it is more a developer facing class, not a user facing class (I am fine with the docs being done as follow up to get this into the release candidate though).
| if precision is not None and precision > 0: | ||
| raise ValueError("precision must be None or 0 for StatevectorEstimator.") |
There was a problem hiding this comment.
| if precision is not None and precision > 0: | |
| raise ValueError("precision must be None or 0 for StatevectorEstimator.") | |
| if precision is None: | |
| precision = 0 |
We don't want to error if a user puts in precision > 0 value for precision (which is a target precision), since we will always exceed it since precision is guaranteed to be 0 by implementation. This would prevent actually being able to use this in any application / algorithm workflow which uses a sensible initial guess for precision which would always be > 0.
There was a problem hiding this comment.
@chriseclectic do you propose to:
- ignore the precision
- artificially add imprecision, as was previously implemented
The argument for (1) is that this is a statevector simulation, so it should always be perfect.
The argument for (2) is that some folks may want to use this implementation to test their ideas, which might include testing that their ideas are robust to finite noise.
There was a problem hiding this comment.
1: in the base class we say that it is a target precision, which i implicitly assumed meant "I want you to return me a mean estimate with standard error <= this value". Maybe we can make wording more explicit.
To add to my lack of documentation comments, this classes doc string should explain how the simulation is done and say that it will always return standard error as zero because it returns exact expectation value means computed from the full state vector.
There was a problem hiding this comment.
But (1) and (2) both satisfy "return me an estimate with standard error <= this value". Sure, (1) does a better estimation job, but 2 technically satisfies the requirements if the sampling is done right.
There was a problem hiding this comment.
Sure, but neither raises an error and makes the estimator unusable in portable workflow like this does
There was a problem hiding this comment.
I agree with @ikkoham : the main point of the StatevectorEstimator is not really to compute expectation values for users, it is to provide a simple reference implementation, and a tool for testing. This is because there are other implementations that are strictly better at computing expectation values: for non-exponential (albeit nisqy) runtime you need to use a runtime backend, and for more performant classical simulations (including Clifford) you can use aer. I realize this is a reversal of some previous comments I made, but I've had a bit more time to think about it now.
So, I am in favour of (2) artificially add imprecision, as was previously implemented because some folks may want to use this implementation to test their ideas, which might include testing that their ideas are robust to finite noise. If some user is in the unlikely situation that they want to call some_application_funcion(estimator: BaseEstimatorV2) with noiseless estimator, and they know that some_application_function internally hard-codes specific precisions >0, they can subclass StatevectorEstimator:
class NoiselessEstimator(StatevectorEstimator):
def run(self, pubs, precision = None):
return super().run(pubs, 0.0)
result = some_application_function(NoiselessEstimator())I do think that the default precision for StatevectorEstimator should be 0, however.
There was a problem hiding this comment.
I don't see why a user should expect this estimator to be noisy unless we tell them that it is. I think we should explicitly state this is an ideal estimator and the precision argument is ignored since it always perfectly estimates the observable in the class documentation.
You need to consider how an estimator is used in computational workflows. If someone is writing an algorithm (say VQE for example) that takes an estimator to run on, it will choose some initial guess for a precision, and then perhaps depending on the variance of the result may modify this processing for subsequent evaluations. If you have some special test Estimator that only works with an argument of precision 0 this will error in these sort of applications. You don't loose anything by return a precision less than requested.
If you want to add noise to this estimator so precision effects the results artificially you can do so (or make a second subclass estimator that adds noise), but i will not approve this PR while it raises an error or warning under a canonical workflow.
There was a problem hiding this comment.
(Also, to be clear, and I think we all three agree on this point: the StatevectorEstimator shouldn't raise errors when given precision>0 in any case. Thanks @chriseclectic for flagging this, I shouldn't have missed that. That would make the thing non-portable, in addition to being annoying. So I'm in particular trying to get to the bottom of choosing between 1 and 2, outlined near the top of this thread)
There was a problem hiding this comment.
If you want the class to add noise for precision > 0 I am fine with that, just dont add a shots option, all you shuold need is a seed field (like sampler) and initialize an rng in run and do something like rng.normal(evs, precision) (im not exactly sure correct syntax for normal distribution sampling in numpy)
There was a problem hiding this comment.
Great, let's go with (2) and implement rng seeding in the same way as the StatevectorSampler. No need to mention shots anywhere.
|
As class name becomes |
Co-authored-by: Takashi Imamichi <[email protected]>
| obs = SparsePauliOp(paulis, coeffs) # TODO: support non Pauli operators | ||
| expectation_value = final_state.expectation_value(obs) | ||
| if precision != 0: | ||
| expectation_value = rng.normal(expectation_value, precision) |
There was a problem hiding this comment.
You need to make sure expectation_value is float to use normal. Otherwise, it raises an error as follows.
In [4]: rng.normal(1j, 0.1)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[4], line 1
----> 1 rng.normal(1j, 0.1)
File numpy/random/_generator.pyx:1220, in numpy.random._generator.Generator.normal()
File _common.pyx:585, in numpy.random._common.cont()
TypeError: float() argument must be a string or a real number, not 'complex'There was a problem hiding this comment.
Thanks. You are right. Actually, I haven't validate this since V1.
There was a problem hiding this comment.
This PR LGTM, but I'm going to wait for @chriseclectic make any final comments about the docstring or other lingering concerns. My preference is to get this merged (overdue) and make final tweaks as needed.
Co-authored-by: Ian Hincks <[email protected]>
Summary
Add EstiamtorV2
This PR includes base classes and StatevectorEstimator (reference implementation).
Details and comments
This PR is based on the following RFCs:
https://github.com/Qiskit/RFCs/blob/master/0015-estimator-interface.md
https://github.com/Qiskit/RFCs/blob/master/0018-primitive-options-type.md
https://github.com/Qiskit/RFCs/blob/master/0017-base-primitive-unification.md
Items not included in this PR:
Note: I might do a force push. → done