qiskit-community / qiskit-algorithms

A library of quantum algorithms for Qiskit.
https://qiskit-community.github.io/qiskit-algorithms/
Apache License 2.0
111 stars 54 forks source link

Add slow_test as it's no longer part of Qiskit 1.0 #141

Closed woodsp-ibm closed 8 months ago

woodsp-ibm commented 8 months ago

Summary

slow_test used to be part of qiskit.test but the whole test folder was removed for Qiskit 1.0. This adds a copy of that slow_test decorator locally.

Additionally this updates other units testing for failures that started since this was created - see comments below for more detail.

Details and comments

The Monitor jobs, that run against Qiskit main branch, started failing last night after the removal.

coveralls commented 8 months ago

Pull Request Test Coverage Report for Build 7788009998


Files with Coverage Reduction New Missed Lines %
qiskit_algorithms/gradients/reverse/reverse_gradient.py 3 94.74%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 7785719773: -0.04%
Covered Lines: 6486
Relevant Lines: 7157

💛 - Coveralls
woodsp-ibm commented 8 months ago

Since I did this it seems there is a new failure in the 1.0 Monitor dues to the change in signature of inverse method. This adds the additional parameter so lint is ok with it in 1.0, the extra parameter is ok with lint in earlier versions e.g 0.46. Rather than create a separate PR as this was to fix the 1.0 monitoring jobs, and it just again test logic change, I included it here.

woodsp-ibm commented 8 months ago

Hmm., now its running tests, which it did not get that far on nightly CI, it seems there are a whole bunch of failures that did not happen a couple of days ago when I first pushed this PR.

woodsp-ibm commented 8 months ago

Seems the failures are due Estimator now only supporting SparsePauliOp in 1.0. So where tests had a SparsePauliOp and then done to_matrix() on this to create a matrix op, and tested with both, I dropped the conversion and test using that.

In one place a different operator was created to check re-use, I converted that to a SparsePauliOp.

Typehints on the algorithms for the operators supported ideally need updating since its no longer BaseOperator as per what the Estimator supported. Maybe this can be revisited along with #136.

woodsp-ibm commented 8 months ago

This looks good to me, maybe we can update the type hints in a quick follow-up PR

I don't know whats the right thing here. If you use 0.46 then the hints are correct as the primitives support a wider set. If you use the new 1.0 then its really only then its just SparsePauliOp. Since most people arguably just use SparsePauliOp - the apps all do - maybe it does not matter too much. If you had code that used one of the other operators it would still work with 0.46 albeit that it raises a deprecation warning rather than fail as it does if you use 1.0. If we changed to SparsePauliOp only then maybe mypy would complain on your code with the other operators.