Skip to content

Conversation

AnirudhDagar
Copy link
Member

@AnirudhDagar AnirudhDagar commented Mar 30, 2022

Reference issue

Closes #15608

What does this implement/fix?

Completely deprecate scipy.misc.

TODO:

  • Replace scipy.stats.rv_continuous and scipy.stats.kolmogn_p usage of misc.derivative

Question

As discussed in the mailing list central_diff_weights and derivative should be deprecated and completely removed in the future. Within SciPy, misc.derivative is used internally only in scipy.stats.rv_continuous here and in scipy.stats._ksstats._kolmogn_p here.

I'm unsure about what is the best way to replace that, since misc.derivative will be deprecated. Any suggestions will be helpful. Thanks in advance!

Additional information

ref #15607

@tylerjereddy tylerjereddy added deprecated Items related to behavior that has been deprecated scipy.misc labels Mar 31, 2022
@rgommers rgommers self-requested a review March 31, 2022 12:01
@h-vetinari
Copy link
Member

Without preference one way or another, just noting that the more we explicitly announce removal of something by scipy 2.0.0, the more we create an expectation for this to arrive reasonably soon (i.e. it would be weird as a user IMO to keep seeing this warning for 5+ years).

Seeing the discussion in #15528, the impression I got is that many people see 2.0 still quite far on the horizon (though not never) compare also the notes in the wiki on this.

In the context of #14360, some modules now already announce a removal by 2.0.0, some don't.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @AnirudhDagar! This looks pretty straightforward, but looks like it should wait till the scipy.datasets PR goes in first.

I'm unsure about what is the best way to replace that, since misc.derivative will be deprecated.

I'd say it's best to just keep using it and suppress the deprecation warning. That decouples this from any functional changes to scipy.stats. When the time comes to remove scipy.misc, the person doing the cleanup can just move derivative to a private function inside stats, if the usage hasn't been cleaned up before then.

Without preference one way or another, just noting that the more we explicitly announce removal of something by scipy 2.0.0, the more we create an expectation for this to arrive reasonably soon

There's no timeline at the moment and little pressure, the "we remove a module that may break a lot of older code" is just for a major version bump, which is all this warning says.

@j-bowhay
Copy link
Member

j-bowhay commented Jul 8, 2022

central_diff_weights and derivative state they will be removed in 2.0.0. ascent, face and electrocardiogram don't have a timeline for execution. I appreciate the whole submodule will be removed at 2.0.0 but without a timeline it is unclear whether these 3 will be left until 2.0.0 or removed after the usual two releases.

@rgommers
Copy link
Member

rgommers commented Jul 8, 2022

Good question - I'd say 2 releases, since they're just moving to scipy.datasets.

@AnirudhDagar
Copy link
Member Author

I guess I should also bump up the deprecation warnings to 1.10 since 1.9 is already underway unless we somehow manage to sneak both these PRs in before 1.9 release which I don't see happening.

@AnirudhDagar
Copy link
Member Author

FAILED ..\..\stats\tests\test_qmc.py::TestPoisson::test_mindist - numpy.core....

Test failure seems unrelated, but I'm curious if it's just a flake or an actual recurring problem in main.

@ngam
Copy link

ngam commented Jul 31, 2022

As discussed in the mailing list central_diff_weights and derivative should be deprecated and completely removed in the future. Within SciPy, misc.derivative is used internally only in scipy.stats.rv_continuous here. I'm unsure about what is the best way to replace that, since misc.derivative will be deprecated. Any suggestions will be helpful. Thanks in advance!

Good question - I'd say 2 releases, since they're just moving to scipy.datasets.

@AnirudhDagar @rgommers is it still the case that derivative and central_diff_weights will be gone soon?

Why remove them completely, can't we just add them to the integrate (for example) subpackage? I was recently thinking of contributing a numerical_derivative counterpart to derivative (or within the same call) but haven't been able to fix one weird edge case... the idea is to do what np.gradient does but with higher accuracy (and possibly higher order)... are there any obvious implementations of that that I am missing? Currently, derivative really needs a callable function

Or we can obviously add a "differentiate" subpackage if there is sufficient use. Differentiation, like integration, are likely pretty important for users and so expanding the usage of derivative can be nice.

@rgommers
Copy link
Member

@ngam yes indeed. This was discussed in this mailing list thread. There are other packages which are much better than these two fairly random functions, so best to point folks to those.

Or we can obviously add a "differentiate" subpackage if there is sufficient use. Differentiation, like integration, are likely pretty important for users and so expanding the usage of derivative can be nice.

This has been discussed on and off for many years now, but no one has actually done the work (a few half-attempts failed). For example, see gh-2035 and gh-7457. At this point I think the topic is dead. If someone writes a high-quality module outside of SciPy and then wants to incorporate it, that can be discussed. But writing from scratch directly in the SciPy repo is no longer an option.

@ngam
Copy link

ngam commented Jul 31, 2022

I see, thanks for the background information and links! The plan (obviously) makes sense.

I have heard about findiff and numdifftools, but I do believe a differentiate/derivatives scipy modulde/subpackage would be a really good addition in the future. I also agree with this:

If someone writes a high-quality module outside of SciPy and then wants to incorporate it, that can be discussed. But writing from scratch directly in the SciPy repo is no longer an option.

If anything nice comes out of my work, I will consider submitting it for review in the future.

Thanks!!

@rgommers
Copy link
Member

This is unblocked now that the scipy.datasets PR is merged.

@AnirudhDagar
Copy link
Member Author

Yes, I believe there is nothing to change in this PR though. Should be ready to go.

@rgommers rgommers added this to the 1.10.0 milestone Aug 12, 2022
@rgommers
Copy link
Member

It looks like this deprecation warning shows up and should be filtered out so the doc build doesn't fail in CI:

DeprecationWarning:     `face` is deprecated!
    scipy.misc.face has been deprecated in SciPy v1.10.0; and will be completely removed in SciPy v1.12.0. Dataset methods have moved into the scipy.datasets module. Use scipy.datasets.face instead.

@AnirudhDagar
Copy link
Member Author

Sorry if this is a trivial question, but I don't know how to ignore the warning from the docstring example. I mean using warnings context manager in the docstring example is definitely not the correct way. I'm sure there is some other way to do that. @rgommers could you please point me to that? Thanks

@rgommers
Copy link
Member

You can add it to filterwarnings in pytest.ini, I'm fairly sure that's the correct way to suppress warnings like these.

@AnirudhDagar
Copy link
Member Author

AnirudhDagar commented Aug 12, 2022

Ok, so I noticed that I forgot about updating multiple other docstring examples that were using scipy.misc still. I've replaced those with scipy.datasets now.

Apartment from that, should I just filter out these two cases with a warnings content manager?

  1. return scipy.misc.derivative(_kk, x, dx=delta, order=5)
  2. return derivative(self._cdf, x, dx=1e-5, args=args, order=5)

We can add a private derivative method later (exact same as scipy.misc.derivative) to preserve this when the misc module is completely gone as discussed before.

@ev-br
Copy link
Member

ev-br commented Aug 12, 2022

Better not add an extra layer of pytest skips. Ideally, remove scipy.misc from the list of submodules to doctest, add scipy.datasets (the latter is a good move regardless of the former) https://github.com/scipy/scipy/blob/main/tools/refguide_check.py#L89.

Alternatively, add a filterwarings filter in refguide-check (gh-16391 has a better way of adding these kinds of filters)

@ev-br
Copy link
Member

ev-br commented Aug 12, 2022

We can add a private derivative method

I'd suggest to copy-paste it to scipy stats as a private function now and use it.

@AnirudhDagar
Copy link
Member Author

AnirudhDagar commented Aug 12, 2022

I'd suggest to copy-paste it to scipy stats as a private function now and use it.

Thanks @ev-br where exactly in scipy.stats would you suggest moving the method to, so that it can be shared for both the above-mentioned cases, perhaps scipy.stats._common.py?

Ideally, remove scipy.misc from the list of submodules to doctest, add scipy.datasets (the latter is a good move regardless of the former)

Datasets submodule is already in the list, it was added in #15607. Removing misc from the list should do the trick.

@AnirudhDagar
Copy link
Member Author

AnirudhDagar commented Aug 23, 2022

Fixed the failing problems. This is ready for review, and I hope CI is also green now.

Ps. Added global deprecation doc warning for the documentation on scipy.misc page.

Edit: Please ignore the lint errors in scipy/stats/_common.py coming from _central_diff_weights and _derivative, those functions are just copy-pasted from misc module as suggested above, other than renaming them (prefixed underscore) to show they are private.

Edit2: refguide failure is probably unrelated.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @AnirudhDagar. Overall this looks good. The main comment I have is that from scipy import misc does not actually raise a deprecation warning. It should, otherwise it's too easy to miss and we are likely to break more code when we actually remove it.

To do this and keep things working now that there's two levels of deprecated public-like names (ex: misc and misc.common), you can copy how this works in for example scipy/sparse/linalg/eigen.py and the __init__.py next to it (that had the same issue with sparse.linalg.eigen and sparse.linalg.eigen.arpack).

@AnirudhDagar
Copy link
Member Author

@rgommers sorry for the delay, I've made the changes as requested.

I was unsure what you meant by the similarity with scipy/sparse/linalg/eigen.py example. Please let me know if you meant something else regarding the deprecation of the whole misc module. Thanks!

@AnirudhDagar
Copy link
Member Author

Gentle ping @rgommers. Request you to review this again :) Thanks!

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @AnirudhDagar, LGTM now. In it goes!

Please let me know if you meant something else regarding the deprecation of the whole misc module.

I actually meant that from scipy import misc by itself should raise a warning. But let's leave it as is for now. It's fairly annoying/disruptive to implement, and what is an easier strategy is to remove all the functions by 1.12.0 and then raise the warning once there are no more tests to worry about.

@rgommers rgommers merged commit 81e636e into scipy:main Sep 5, 2022
@rgommers
Copy link
Member

rgommers commented Sep 5, 2022

@AnirudhDagar could you add an entry for this in https://github.com/scipy/scipy/wiki/Release-Note-Entries-for-SciPy-1.10.0?

@j-bowhay
Copy link
Member

@h-vetinari there are a few things here slated for removal in 1.12.0 could you add them to the tracker please

@h-vetinari
Copy link
Member

@h-vetinari there are a few things here slated for removal in 1.12.0 could you add them to the tracker please

Hey @j-bowhay, sorry I've been MIA for quite a while. 😑

I now added scipy.misc to the tracker (and tried to go through my remaining github notification emails) - please let me know if I missed something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated Items related to behavior that has been deprecated scipy.datasets scipy.misc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEP: Remove scipy.misc submodule
7 participants