qiskit
qiskit copied to clipboard
Qiskit
Fix Errors in Primitives
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:
- @Qiskit/terra-core
- @ajavadia
- @ikkoham
- @levbishop
- @t-imamichi
It seems that there are some fidelity tests for mismatched left and right circuits that are testing the sampler usage by fidelity to presumably ensure its catching the expected error when it calls run. https://github.com/Qiskit/qiskit-terra/blob/4ac8b24af1cf6f99a418b5f118c29661f89a2b2c/test/python/algorithms/state_fidelities/test_compute_uncompute.py#L141 While the job.result() call is in a try catch block where it re-raises the exception, the call run, like the gradients is not, so this change affected the test. It seems more like its testing the sampler than the fidelity as such with this error checking though - bit since the fidelity has none itself on the run and relies on this I guess it checks the overall behavior is as expected. @ElePT FYI
It seems that there are some fidelity tests for mismatched left and right circuits that are testing the sampler usage by fidelity to presumably ensure its catching the expected error when it calls run.
https://github.com/Qiskit/qiskit-terra/blob/4ac8b24af1cf6f99a418b5f118c29661f89a2b2c/test/python/algorithms/state_fidelities/test_compute_uncompute.py#L141
While the job.result() call is in a try catch block where it re-raises the exception, the call run, like the gradients is not, so this change affected the test. It seems more like its testing the sampler than the fidelity as such with this error checking though - bit since the fidelity has none itself on the run and relies on this I guess it checks the overall behavior is as expected. @ElePT FYI
I see. Given that as Steve mentioned, these errors are actually handled by the Sampler and not ComputeUncompute, I can remove this test from the fidelity module. I see 2 options:
- I have an open PR addressing a few bug fixes in the fidelity classes #8755, it's a bit messy but I could include the fix there.
- If there is no rush to include this PR in the next release, I could open a quick bug-fix PR for this purpose.
Whichever you prefer :)
TBH, I don't have a strong opinion to this issue. We don't have any major rules about Qiskit errors... Since this is an API breaking, I would like to change it before next release.
Pull Request Test Coverage Report for Build 3125421036
- 14 of 27 (51.85%) changed or added relevant lines in 2 files are covered.
- 5 unchanged lines in 2 files lost coverage.
- Overall coverage decreased (-0.005%) to 84.415%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| qiskit/primitives/base_sampler.py | 6 | 11 | 54.55% |
| qiskit/primitives/base_estimator.py | 8 | 16 | 50.0% |
| <!-- | Total: | 14 | 27 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| qiskit/extensions/quantum_initializer/squ.py | 2 | 79.78% |
| qiskit/pulse/library/waveform.py | 3 | 91.49% |
| <!-- | Total: | 5 |
| Totals | |
|---|---|
| Change from base Build 3116583352: | -0.005% |
| Covered Lines: | 59592 |
| Relevant Lines: | 70594 |
💛 - Coveralls
Well, you already fixed the CI but decided to remove the test anyway to prevent future situations like this. It's a bit trivial, so if you could quickly review it, it could be merged with this fix :)