Exorcise deprecated sparse matrix (scipy 2.0) - #20167

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.

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.

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

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!

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.

I mean pick up where you left off and conduct following iterations. I thought that's what you were asking for. Maybe I misunderstood ?

I am waiting for @kbwestfall to do a careful read of the migration guide, since he wrote the original code. :D

FWIW I'm used to ready migration guides for code I didn't write, so I should be comfortable filling in for Kyle if need be :)

@neutrinoceros do you want to review then? Feel free. Thanks!

sign me up. I'll do this in the morning.

Sorry that I dropped off the map for this one. Two quick responses (which may be too hasty...):

• I'm not sure I agree that the new approach is not internally consistent. Eventually, we would deprecate the use of old versions of scipy that provide, e.g., coo_matrix. And I expect this will include the new bespoke definition of isspmatrix_csr or some version of it. So everything currently in the relevant else block would be kept.
• I was hoping to dig more into the differences between csr_matrix and csr_array to propose a solution, but I'm happy for @neutrinoceros to do this in a different PR.

Eventually, we would deprecate the use of old versions of scipy that provide, e.g., coo_matrix

that's my point though: here it's dropped immediately, without a deprecation cycle, so it's a breaking change instead of a backward compatibility layer

Then why does test_scipy_funcs exist ?

Eventually, we would deprecate the use of old versions of scipy that provide, e.g., coo_matrix

that's my point though: here it's dropped immediately, without a deprecation cycle, so it's a breaking change instead of a backward compatibility layer

Maybe I'm misunderstanding, but I don't think this is dropping anything that the user's scipy version supports. I.e., if they have scipy < 2.0, the code will import and use coo_matrix but alias it to coo_array.

The test_scipy_funcs is just to ensure that the code succeeds or fails as expected. I.e., the scipy aliases should always be callable functions whether or not scipy is available, but they should report ModuleNotFoundError if scipy is not installed. I don't understand how this connects to the deprecation issue. Are you worried that users might have been doing something like from astropy.nddata.covariance import coo_matrix? (I would hope users are not doing this...)

I think it's more important to understand whether or not coo_matrix and coo_array can be treated as interchangeable within the context of this module.

Are you worried that users might have been doing something like from astropy.nddata.covariance import coo_matrix?

Yes. The fact that we have a test exercising it seems to indicate that this is a supported use case. If that is in fact not its purpose, I don't understand what it's there for.

(I would hope users are not doing this...)

Me too, but the existing code seems ambiguous wether this was intentionally supported or not, especially because there are 0 indications that these functions are considered private.

Are you worried that users might have been doing something like from astropy.nddata.covariance import coo_matrix?

Yes. The fact that we have a test exercising it seems to indicate that this is a supported use case. If that is in fact not its purpose, I don't understand what it's there for.

This is a low-level test to ensure that the scipy imports within the covariance module are appropriately dealt with, not a test that exercises user-facing functionality. If that means it's not a test that we should have, I'm fine with removing it.

(I would hope users are not doing this...)

Me too, but the existing code seems ambiguous wether this was intentionally supported or not, especially because there are 0 indications that these functions are considered private.

To me, this is like having a foo.py file that includes import numpy as np, and then someone doing from foo import np. Do we need to guard against that? If removing test_scipy_funcs avoids this question, then that's another reason I would be happy to remove it.

If that means it's not a test that we should have, I'm fine with removing it.

I don't think it's worth having, but having an inline explanation would solve the biggest problem I have with it: confusion.

To me, this is like having a foo.py file that includes import numpy as np, and then someone doing from foo import np.

I think you're implying that it's a bad idea, and I agree ! Nonetheless, it is sometimes done deliberately and I was under the impression that it was the case here.

Do we need to guard against that?

we can't really guard against it, though a module level __all__ (and possibly a module level __export__ (PEP 842, on going discussion) would clarify the intent.

If removing test_scipy_funcs avoids this question, then that's another reason I would be happy to remove it.

kill it with fire then 🔥

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

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?

I have rebased and removed test_scipy_funcs. Thanks, all!

OK now devdeps only have 1 failure from #20231 that is unrelated to scipy

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.

I rebased your branch. I think it now addresses the only remaining failures on allowed-deps, so we should be able to see what's up with just the global status on this job now.