Deprecate PauliList estimator observables
#11055
Conversation
Deprecates using a `PauliList` as an observable that is implicitly converted to a `SparsePauliOp` with coefficients 1 when calling `Estimator.run`. Users should instead explicitly convert the argument using `SparsePauliOp(pauli_list)` first.
chriseclectic
requested review from
ikkoham,
t-imamichi,
a team,
woodsp-ibm and
ElePT
as code owners
|
One or more of the the following people are requested to review this:
|
chriseclectic
added
the
Changelog: Deprecation
| @classmethod | ||
| def setUpClass(cls): | ||
| super().setUpClass() | ||
| allow_dep_warning_message = [ | ||
| r"Implicit conversion from a PauliList to a SparsePauliOp*", | ||
| ] | ||
| for msg in allow_dep_warning_message: | ||
| warnings.filterwarnings("default", category=DeprecationWarning, message=msg) |
There was a problem hiding this comment.
- Can we add a comment explaining why this needs to be a warning skip and the tests/library cannot be fixed, including the criteria for when the skip can be removed? It could also do with a
moduleargument setting to limit the skip to precisely the allowed code, not allowed in general in all code. - This skip is never torn down again but affects the global state of the
warningsmodule, so it actually applies to all test cases, becauseqiskit.algorithmstests run first alphabetically.
There was a problem hiding this comment.
Since the algorithms will be removed, how about merging this PR after that?
#11086
There was a problem hiding this comment.
Point 1 is still important because Algorithms isn't disappearing completely, it's just in a separate package. We should know how much work it is that we're asking downstream packages to do to manage a deprecation, and if the only option is a warning skip (though I don't think that is the case here), that's an indication that there are use cases that the new form cannot supply. It's a good thing that this warning was triggered before Algorithms moved out and we'd have seen it too late.
The warnings and errors aren't there to be annoying, they're there to ensure that the effect of deprecations on consumers is properly accounted for, and skipping a warning that we emitted is very rarely the correct action.
There was a problem hiding this comment.
@chriseclectic I don't recall any algorithm/test in qiskit.algorithms explicitly using PauliList, but I guess that this warning filter was introduced after some tests raised warnings. Can you point out which tests did? This would help address Jake's first point.
There was a problem hiding this comment.
I expected that used around opflow, but not explicitly by algorithms. Searching here shows PauliList in 8 test files, one in opflow, nothing explicit in algorithms. Code use in primitives, visualization and of course quantum_info modules. Searching qiskit_algorithms repo shows no occurrence of PauliList at all
There was a problem hiding this comment.
I ran the test locally and found that no algorithm tests raise the deprecation warning due to PauliList. The deprecation warning is raised by only test.python.primitives.test_estimator.TestObservableValidation.test_validate_observables_deprecated_{1,2} as expected. So, I don't think filterwarnings is necessary.
There was a problem hiding this comment.
Right. I ran the test locally and all tests passed.
Co-authored-by: Takashi Imamichi <[email protected]>
* Deprecate PauliList estimator observables Deprecates using a `PauliList` as an observable that is implicitly converted to a `SparsePauliOp` with coefficients 1 when calling `Estimator.run`. Users should instead explicitly convert the argument using `SparsePauliOp(pauli_list)` first. * Revert algorithms_test_case.py * Update test/python/algorithms/algorithms_test_case.py Co-authored-by: Takashi Imamichi <[email protected]> * Update test/python/primitives/test_estimator.py --------- Co-authored-by: Takashi Imamichi <[email protected]> Co-authored-by: ikkoham <[email protected]>
* Deprecate PauliList estimator observables Deprecates using a `PauliList` as an observable that is implicitly converted to a `SparsePauliOp` with coefficients 1 when calling `Estimator.run`. Users should instead explicitly convert the argument using `SparsePauliOp(pauli_list)` first. * Revert algorithms_test_case.py * Update test/python/algorithms/algorithms_test_case.py Co-authored-by: Takashi Imamichi <[email protected]> * Update test/python/primitives/test_estimator.py --------- Co-authored-by: Takashi Imamichi <[email protected]> Co-authored-by: ikkoham <[email protected]> Co-authored-by: Christopher J. Wood <[email protected]>
1ucian0
added
Changelog: None
* Deprecate PauliList estimator observables (#11055) * Deprecate PauliList estimator observables Deprecates using a `PauliList` as an observable that is implicitly converted to a `SparsePauliOp` with coefficients 1 when calling `Estimator.run`. Users should instead explicitly convert the argument using `SparsePauliOp(pauli_list)` first. * Revert algorithms_test_case.py * Update test/python/algorithms/algorithms_test_case.py Co-authored-by: Takashi Imamichi <[email protected]> * Update test/python/primitives/test_estimator.py --------- Co-authored-by: Takashi Imamichi <[email protected]> Co-authored-by: ikkoham <[email protected]> Co-authored-by: Christopher J. Wood <[email protected]> * documentation * typo in warning msg * adapt test --------- Co-authored-by: Takashi Imamichi <[email protected]> Co-authored-by: ikkoham <[email protected]> Co-authored-by: Christopher J. Wood <[email protected]>
Summary
Deprecates using a
PauliListas an observable that is implicitly converted to aSparsePauliOpwith coefficients 1 when callingEstimator.run. Users should instead explicitly convert the argument usingSparsePauliOp(pauli_list)first.Details and comments