Skip to content

Exorcise deprecated sparse matrix (scipy 2.0) - #20167

Closed
pllim wants to merge 4 commits into
astropy:mainfrom
pllim:scipy-took-red-pill
Closed

Exorcise deprecated sparse matrix (scipy 2.0)#20167
pllim wants to merge 4 commits into
astropy:mainfrom
pllim:scipy-took-red-pill

Conversation

@pllim

@pllim pllim commented Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Description

This pull request would need someone familiar with the nddata code and also who has time to read https://docs.scipy.org/doc/scipy/reference/sparse.migration_to_sparray.html carefully to review.

Reviewers should ignore the one ufunc failure in devdeps that is being handled separately in #20132

Fixes #20164

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@pllim pllim added this to the v7.2.3 milestone Jul 30, 2026
@pllim pllim added no-changelog-entry-needed numpy-dev backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x labels Jul 30, 2026
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim
pllim requested review from bmorris3 and mwcraig July 30, 2026 16:14
@pllim
pllim marked this pull request as ready for review July 30, 2026 16:26
@pllim
pllim requested review from eteq and kbwestfall July 30, 2026 16:27
@pllim

This comment was marked as outdated.

Sign in to view
neutrinoceros
neutrinoceros previously requested changes Jul 30, 2026
View reviewed changes
Comment thread astropy/nddata/covariance.py Resolved
neutrinoceros
neutrinoceros reviewed Jul 30, 2026
View reviewed changes
Comment thread astropy/nddata/tests/test_covariance.py Resolved
neutrinoceros
neutrinoceros reviewed Jul 30, 2026
View reviewed changes
Comment thread astropy/nddata/covariance.py Resolved
@pllim

This comment was marked as resolved.

Sign in to view
kbwestfall
kbwestfall reviewed Jul 30, 2026
View reviewed changes

@kbwestfall kbwestfall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes with the caveat that I've only briefly looked at the migration page. I can dig in more later.

Do we know when this deprecation will actually happen? The warning at the top of this page just says it will be done "in the next few releases".

Comment thread astropy/nddata/covariance.py Resolved
Comment thread astropy/nddata/covariance.py Resolved
@neutrinoceros

neutrinoceros commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Do we know when this deprecation will actually happen? The warning at the top of this page just says it will be done "in the next few releases".

Scipy usually removes deprecated APIs after just a couple minor releases.

@pllim
pllim force-pushed the scipy-took-red-pill branch 2 times, most recently from 2ef79cb to ec1d1fd Compare July 30, 2026 17:52
@pllim

pllim commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

scipy might also change more API than this but we can only react to what we see in CI.

@pllim
pllim dismissed neutrinoceros’s stale review July 30, 2026 17:57

Code of concern no longer exist

@pllim
pllim force-pushed the scipy-took-red-pill branch from ec1d1fd to 113b589 Compare July 30, 2026 18:13
@pllim

This comment was marked as outdated.

Sign in to view
mhvk
mhvk reviewed Jul 30, 2026
View reviewed changes
Comment thread astropy/nddata/covariance.py Outdated
if SCIPY_LT_2_0:
from scipy.sparse import coo_matrix, csr_matrix, find, isspmatrix_csr, triu
else:
from scipy.sparse import coo_array as coo_matrix

@mhvk mhvk Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indeed should be checked by the nddata folks, most importantly to check no multiplication is used as matrix multiplication, but FWIW, I would tend to flip this around, i.e., turn towards the future by replacing coo_matrix with coo_array in the code and above import coo_matrix as coo_array.

@pllim pllim Jul 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to keep the diff to a minimal but I can flip the naming if y'all want. More code to review though.

@kbwestfall kbwestfall Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had the same thought as Marten. Up to you, but I'm happy to review the changes either way.

@pllim pllim Jul 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK sure, I can flip the naming

@pllim pllim Jul 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I flipped names in the code, but I left the in-code comments and docstrings alone. Cannot really touch the docstring or intersphinx would break.

@pllim
pllim force-pushed the scipy-took-red-pill branch 2 times, most recently from 2c76831 to b3bae88 Compare August 3, 2026 18:46
@pllim

pllim commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Any chance someone can read the guide carefully soon (in a few days)? If not, I will temporarily ignore these warnings instead so we don't let even more failures creep into devdeps. Thanks!

@neutrinoceros

neutrinoceros commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

I'm paying close attention to devdeps and will be careful not to let new ones through. I can also take over this PR if you like.

@pllim

pllim commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

I am getting confused. Are we staying with this PR or moving to a new one by @neutrinoceros ?

@kbwestfall

kbwestfall commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

I am getting confused. Are we staying with this PR or moving to a new one by @neutrinoceros ?

My understanding is that we're sticking with this one. The remaining items (as far as I understand) are:

  • I still need to understand the differences between, e.g., csr_matrix and csr_array. Or, @neutrinoceros, do you already have a handle on this?
  • We want to remove the test_scipy_funcs test function. @pllim , can you do that in your branch?

@pllim
pllim force-pushed the scipy-took-red-pill branch from b3bae88 to a1640e8 Compare August 10, 2026 21:48
@pllim

pllim commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

I have rebased and removed test_scipy_funcs. Thanks, all!

@pllim
pllim dismissed neutrinoceros’s stale review August 10, 2026 21:52

The test of concern is removed

@pllim

This comment was marked as resolved.

Sign in to view
@neutrinoceros

neutrinoceros commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

I still need to understand the differences between, e.g., csr_matrix and csr_array. Or, @neutrinoceros, do you already have a handle on this?

Not yet. I'm not on vacation but still traveling so my capacity is limited these days (but I'm hoping to ramp it up in the next couple days). I can prioritize this on request, but until then it's only somewhat near the top of my list.

@pllim pllim mentioned this pull request Aug 13, 2026
Merged
1 task
@neutrinoceros

This comment was marked as resolved.

Sign in to view
@pllim

This comment was marked as outdated.

Sign in to view
@pllim pllim mentioned this pull request Aug 16, 2026
Merged
1 task
@pllim
pllim force-pushed the scipy-took-red-pill branch from da11fe8 to 9ff5b54 Compare August 18, 2026 14:11
@neutrinoceros
neutrinoceros self-requested a review August 18, 2026 15:53
@neutrinoceros

neutrinoceros commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

I'll try to get back to reviewing this before the weekend. Thanks for your patience !

pllim and others added 4 commits October 15, 2026 17:27
@pllim @kbwestfall
TST: First pass at exorcising spmatrix
adffca9 
but did not read conversion guide carefully

Co-authored-by: Kyle Westfall <[email protected]>
@pllim
Use new names instead
1f55834 
but leave in-code comments and docstrings alone
@pllim
TST: Remove test_scipy_funcs
8248ffe 
because it is confusing to test API of another library as our own
@pllim
Revert astropy#20245
006b68e
@pllim
pllim force-pushed the scipy-took-red-pill branch from 22a0b0d to 006b68e Compare October 15, 2026 21:27
@pllim

This comment was marked as resolved.

Sign in to view
@pllim

pllim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

@neutrinoceros and @kbwestfall , devdeps is green here now. Are you able to re-review? Thanks!

@astrofrog astrofrog modified the milestones: v7.2.3, v7.2.4 Oct 16, 2026
@neutrinoceros

neutrinoceros commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

on it

@neutrinoceros neutrinoceros mentioned this pull request Oct 19, 2026
Open
1 task
@neutrinoceros

neutrinoceros commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

I finally had an hour to sit through this (sorry for the delay !) and I found a bunch of reasons why I think this simple approach isn't viable. I opened #20647 with my findings and proposed plan for action. As a consequence, I also advise closing this PR.

@pllim

pllim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Sure. Thanks!

@pllim pllim closed this Oct 19, 2026
@pllim
pllim deleted the scipy-took-red-pill branch October 19, 2026 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk left review comments
@kbwestfall kbwestfall kbwestfall left review comments
@mwcraig mwcraig Awaiting requested review from mwcraig
@bmorris3 bmorris3 Awaiting requested review from bmorris3
@eteq eteq Awaiting requested review from eteq
@neutrinoceros neutrinoceros Awaiting requested review from neutrinoceros neutrinoceros is a code owner

Assignees

No one assigned

Labels

backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x nddata no-changelog-entry-needed numpy-dev

Projects

None yet

Milestone

v7.2.4

Development

Successfully merging this pull request may close these issues.

TST: isspmatrix_csr is deprecated in scipy-dev

5 participants

@pllim @neutrinoceros @kbwestfall @mhvk @astrofrog