Skip to content
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

Remove numpy 1.25 constraint #83

Merged
merged 3 commits into from
Oct 9, 2023
Merged

Remove numpy 1.25 constraint #83

merged 3 commits into from
Oct 9, 2023

Conversation

woodsp-ibm
Copy link
Member

@woodsp-ibm woodsp-ibm commented Oct 5, 2023

Summary

Closes #10

This removes the numpy constraints to < 1.25. The failure noted in #10 was the treatment of deprecation warnings as errors by the test base class brought over when the code was migrated here. That base class was changed. Deprecation warnings are now simply collected by CI and should be remedied, if caused by qiskit-algorithms code, by do not cause errors especially if these are emitted by 3rd party libraries. Running test_imfil indeed causes some deprecation messages to be emitted but code still works for now and passes the unit test.

.../SQImFil/_history.py:27: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.)
  fvalhist = float(fval)
.../SQImFil/_history.py:30: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.)
  histout.append([fcount, fvalhist, npgrad, stepn, iarm] + list(map(float, x)))
.../SQImFil/_exec_functions.py:156: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.)
  alist[i] = (x[i] > obounds[i,0]+epsb) and (x[i] < obounds[i,1]-epsb)

When I was looking at the code I noticed the snobfit.py numpy version was how things were in the unit tests before #70 was done. So I updated this file along with the change which was basically to remove the constraint for numpy from contraints.txt. This left the file empty and while removing it completely would be an option I chose to leave it with a single line comment in there.

Details and comments

Given this closes #10 I will create another issue to consider removing the scikit-quant optimizers here as the repo seems pretty much unmaintained with last activity 2 years ago and the snobfit issue /scikit-quant/scikit-quant#24, for which unit tests here are skipped, has been open and without any activity since it was created at the end of last year . Unlike when these optimizer "wrappers" were added, to conform to the base Optimizer class, there is now also Minimizer protocol and the optimizers can still be used directly via that say with a partial.

Update: Created following new issue for scikit-quant optimizers as outlined above

@woodsp-ibm
Copy link
Member Author

woodsp-ibm commented Oct 5, 2023

Seems the mypy ignore in spsa is needed in Python 3.8 when the newest numpy it can find is 1.24.2, but for 3,9 and above, now that the constraint if gone, it gets 1.26 where the ignore is not needed and CI fails with a message to that effect. I altered the code a little for the first parameter - which seems to do the trick locally for me and keep mypy happy whether using numpy 1,24 or 1.26 without the need for an ignore.

@coveralls
Copy link

coveralls commented Oct 5, 2023

Pull Request Test Coverage Report for Build 6456773406

  • 0 of 2 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 90.529%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_algorithms/optimizers/snobfit.py 0 1 0.0%
qiskit_algorithms/optimizers/spsa.py 0 1 0.0%
Totals Coverage Status
Change from base Build 6456707071: 0.01%
Covered Lines: 6452
Relevant Lines: 7127

💛 - Coveralls

Copy link
Collaborator

@ElePT ElePT left a 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'd also be in favor of removing sckit-quant-based optimizers, I don't really see the point of maintaining these wrappers if the library hasn't been updated in such a long time and they could be still be used through the Minimizer interface. Maybe @Cryoris has some other opinion in the matter

@ElePT ElePT added the automerge label Oct 9, 2023
@mergify mergify bot merged commit 57d21b9 into qiskit-community:main Oct 9, 2023
15 checks passed
@woodsp-ibm woodsp-ibm deleted the numpy_constraint branch October 9, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address numpy<1.25 constraint
3 participants