-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix sparse diag by upgrading SciPy and Numpy and dropping Python 3.9 #650
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #650 +/- ##
=======================================
Coverage 93.45% 93.45%
=======================================
Files 64 64
Lines 4993 4993
=======================================
Hits 4666 4666
Misses 327 327 ☔ View full report in Codecov by Sentry. |
It doesn't make sense to defer this essential fix to align with Python's release schedule. I am good with you removing v3.9 (just make a note in the PR title as well maybe?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, @maximelucas! Good to squash and merge!
Agreed. I also upgraded NumPy as required by SciPy. This was an important bug to fix, but for some reason I only ever noticed it in "email-enron", and even then, only the smaller eigenvalues were affected. So hopefully, the bug hasn't had too much of an impact. |
Fixes #649.
The problems seems to be due to a bug from SciPy that made
setdiag()
not set the whole diagonal to 0 in some cases for sparse arrays. It's been fixed by this PR since, and included in the 1.15 SciPy release.I just upped our SciPy version in the requirements and added a test.