Deprecate implicit Estimator conversion to SparsePauliOp #11535
Conversation
Deprecate implicit conversion from a dense operator to a SpasePauliOp in Esimtator
|
One or more of the the following people are requested to review this:
|
|
Sorry, for main, we should remove these features. |
| @@ -70,7 +68,7 @@ def setUp(self): | |||
| self.fidelity = ComputeUncompute(Sampler()) | |||
| self.betas = [50, 50] | |||
|
|
|||
| @data(H2_PAULI, H2_OP, H2_SPARSE_PAULI) | |||
| @data(H2_PAULI, H2_SPARSE_PAULI) | |||
There was a problem hiding this comment.
I am not too happy with not testing Operator here (adding these tests unvelied a series of bugs in the eigensolver classes post-primitives-migration). What we do in these cases is not remove the test that raises deprecation warnings, but assert that the warnings are raised when using deprecated classes. This is to confirm that the deprecation warning is raised where it is expected (and the motivation behind making tests fail with deprecation warnings).
In this case, you would need to do a type check of "op" to make sure that you are only asserting the warning when it is of type Operator.
I know that these are deprecated classes, but until they are removed, people are still able to use them, and making sure they work I think is still important.
|
For the VQD tests etc, that are not directly testing an Estimator, I would have thought swallowing the DeprecationWarning would have sufficed. Asserting you see a warning is more testing the underlying function is emitting it or not. At the VQD test level arguably all we care is it works (for now). Since all this lot is removed already in main and this is just for the one release maybe it does not matter so much. |
|
Seems to need to add |
|
Done |
There was a problem hiding this comment.
Thanks for applying my suggestion @ikkoham. Annoyingly, sampling vqe is not really using the Estimator but still raises the warning because it inherits from BaseEstimator. Technically the ideal thing would be to keep the test and filter the warnings, but I don't think this particular class did any special handling of the operator depending on the type, so it's probably not the worst not to have that test (given the deprecated status). I'll approve and leave some time for final opinions before adding to the merge queue.
Summary
Port of #11056 to stable/0.46 branch
Details and comments