-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 better error messages for typical SamplerV2 and EstimatorV2 error cases #12031
Conversation
One or more of the the following people are requested to review this:
|
151db35
to
ff915f0
Compare
Pull Request Test Coverage Report for Build 8344800410Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
Thanks a lot for this PR, I think it makes a lot of sense. I added a suggestion to make the message even more clear (specifying single circuit). Would it make sense to extend this to the Estimator
too? I am not sure if there is also a case with a single circuit and observable where this type of issue would happen.
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
36eeca0
to
0fad7ed
Compare
I made the following changes since the last review comment.
|
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.
Thanks a lot! I agree that exposing the pub could make the message too long, so using the type is a good call. It might still be a bit unexpected for users to see a report of <class 'qiskit._accelerate.quantum_circuit.CircuitInstruction'>
(for example) as their input, but I think that overall the message is much much clearer and hopefully the suggestion about adding [ ]
covers a lot of these initial user errors. Mostly LGTM (I left a tiny tiny comment). The changes to the test seeds/rng also look good.
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.
Sorry, one more question, would a single sampler pub input like sampler.run((circuit,parameters))
run successfully? or would it also have to be wrapped in []
like the estimator one (sampler.run([(circuit,parameters)])
)? Asking in case this also needs to be covered by the error messages.
|
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
I see... it's difficult to tell apart both cases to have a specific message for each case. What do you think if we also add a second part to the error message in
|
Thank you for your suggestion. I added your additional message if the number of parameters do not match and |
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.
Thanks a lot for your changes, I think this looks very good now. Approving!
… cases (#12031) * add an early error for a case * Update qiskit/primitives/containers/sampler_pub.py Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> * add better message for estimator pub * Update qiskit/primitives/containers/estimator_pub.py Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> * add an additional message --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> (cherry picked from commit 194cc8f) # Conflicts: # test/python/primitives/test_backend_estimator_v2.py # test/python/primitives/test_backend_sampler_v2.py
… cases (backport #12031) (#12041) * Add better error messages for typical SamplerV2 and EstimatorV2 error cases (#12031) * add an early error for a case * Update qiskit/primitives/containers/sampler_pub.py Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> * add better message for estimator pub * Update qiskit/primitives/containers/estimator_pub.py Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> * add an additional message --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> (cherry picked from commit 194cc8f) # Conflicts: # test/python/primitives/test_backend_estimator_v2.py # test/python/primitives/test_backend_sampler_v2.py * remove tests for backend primitives V2 because they will appear for 1.1 --------- Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com> Co-authored-by: Takashi Imamichi <imamichi@jp.ibm.com>
Summary
If users want to run a circuit with SamplerV2, they need to call
SamplerV2.run([circuit])
according to the spec.qiskit/qiskit/primitives/base/base_sampler.py
Lines 166 to 167 in 00b0952
But, they may miss
[]
but the corresponding error message was not comprehensive as follows. This PR detects such a case and shows more comprehensive message.output
main branch:
this PR:
I also added a similar error message for Estimator V2 as suggested by @ElePT.
output
main branch
this PR
Details and comments
In addition to the error messages, I fixed random seed for primitives tests (replaced
np.random.rand
withnp.random.default_rng
).