-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
DEP: Deprecate scipy.misc in favour of scipy.datasets #15901
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
Conversation
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. |
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.
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.
|
Good question - I'd say 2 releases, since they're just moving to |
I guess I should also bump up the deprecation warnings to |
Test failure seems unrelated, but I'm curious if it's just a flake or an actual recurring problem in |
@AnirudhDagar @rgommers is it still the case that Why remove them completely, can't we just add them to the integrate (for example) subpackage? I was recently thinking of contributing a 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. |
@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.
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. |
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 anything nice comes out of my work, I will consider submitting it for review in the future. Thanks!! |
This is unblocked now that the |
Yes, I believe there is nothing to change in this PR though. Should be ready to go. |
It looks like this deprecation warning shows up and should be filtered out so the doc build doesn't fail in CI:
|
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 |
You can add it to |
Ok, so I noticed that I forgot about updating multiple other docstring examples that were using Apartment from that, should I just filter out these two cases with a warnings content manager?
We can add a private |
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) |
I'd suggest to copy-paste it to scipy stats as a private function now and use it. |
Thanks @ev-br where exactly in
Datasets submodule is already in the list, it was added in #15607. Removing misc from the list should do the trick. |
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 Edit: Please ignore the lint errors in Edit2: refguide failure is probably unrelated. |
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.
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
).
@rgommers sorry for the delay, I've made the changes as requested. I was unsure what you meant by the similarity with |
Gentle ping @rgommers. Request you to review this again :) Thanks! |
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.
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.
@AnirudhDagar could you add an entry for this in https://github.com/scipy/scipy/wiki/Release-Note-Entries-for-SciPy-1.10.0? |
@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 |
Reference issue
Closes #15608
What does this implement/fix?
Completely deprecate
scipy.misc
.TODO:
scipy.stats.rv_continuous
andscipy.stats.kolmogn_p
usage ofmisc.derivative
Question
As discussed in the mailing list
central_diff_weights
andderivative
should be deprecated and completely removed in the future. Within SciPy,misc.derivative
is used internally only inscipy.stats.rv_continuous
here and inscipy.stats._ksstats._kolmogn_p
here.I'm unsure about what is the best way to replace that, sinceThanks in advance!misc.derivative
will be deprecated. Any suggestions will be helpful.Additional information
ref #15607