Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add reference implementation of EstimatorV2 #11227
Add reference implementation of EstimatorV2 #11227
Changes from 60 commits
0e428a9
5564603
14c3ff3
4698d74
a074c82
a3d7643
c18b209
cc27a64
9d59318
556aec2
2325224
200ef36
a679f17
55122f4
9ffda4e
8cfc8ff
18e7395
cd5b70f
f075b4e
90447be
18053bd
998547e
c3fd89a
2bc2655
dfcc996
ce01d8f
199aeaa
388d2bf
7777dea
5d320e9
5307922
bcdf842
b6d9376
48bddad
3066fb1
4bbb67f
321cd14
db9e22a
63ed918
3db0063
e0101f4
9046da8
75ca51c
dea7b3b
a5d1e55
b2b74e4
3d694ac
4d917c6
c12bb3c
8387f27
5bb3833
1d37ddb
a095154
bfaf4c0
105551d
47b2302
71a814a
1aa8e4a
d37ce1b
c2c97dd
8e26f18
49a1275
f30debb
10648a7
00a7acf
98af76c
a128546
dbf4363
5d2ae1b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chriseclectic do you propose to:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but neither raises an error and makes the estimator unusable in portable workflow like this does
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 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
becausesome 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 callsome_application_funcion(estimator: BaseEstimatorV2)
with noiseless estimator, and they know thatsome_application_function
internally hard-codes specific precisions >0, they can subclassStatevectorEstimator
:I do think that the default precision for
StatevectorEstimator
should be 0, however.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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, to be clear, and I think we all three agree on this point: the
StatevectorEstimator
shouldn't raise errors when givenprecision>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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 likerng.normal(evs, precision)
(im not exactly sure correct syntax for normal distribution sampling in numpy)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.
Great, let's go with (2) and implement rng seeding in the same way as the
StatevectorSampler
. No need to mention shots anywhere.