Skip to content
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

DEP: scipy.stats.morestats: clarify deprecation warnings #18649

Merged
merged 6 commits into from Jul 4, 2023

Conversation

mdhaber
Copy link
Contributor

@mdhaber mdhaber commented Jun 9, 2023

Reference issue

Toward gh-17572
Toward gh-17771
Toward gh-18279

What does this implement/fix?

  1. BUG: Deprecation warning says to use non-existent symbols #17572 and BUG: cannot import ODEintWarning from scipy.integrate #17771 report inaccurate deprecation warnings. An equivalent example from scipy.stats.morestats is:
from scipy.stats.morestats import wilcoxon  # message is appropriate
# DeprecationWarning: Please use `wilcoxon` from the `scipy.stats` namespace, 
# the `scipy.stats.morestats` namespace is deprecated.

from scipy.stats import wilcoxon  # import works as expected

from scipy.stats.morestats import WilcoxonResult  # message is inaccurate
# DeprecationWarning: Please use `WilcoxonResult` from the `scipy.stats` namespace, 
# the `scipy.stats.morestats` namespace is deprecated.

from scipy.stats import WilcoxonResult  # this fails, as it should
# ImportError: cannot import name 'WilcoxonResult' from 'scipy.stats'

The problem is that the deprecation warning logic does not distinguish between:

  • public attributes that are now to be imported only from the subpackage
    (e.g. scipy.stats.morestats.wilcoxon should only be imported as scipy.stats.wilcoxon)
  • private attributes that are not supposed to be imported at all
    (e.g. scipy.stats.morestats.WilcoxonResult was never public, and should no longer be imported at all)

A solution is to distinguish between the two. With this PR:

from scipy.stats.morestats import WilcoxonResult

# DeprecationWarning: `scipy.stats.morestats.WilcoxonResult` is deprecated along with the 
# `scipy.stats.morestats` namespace. 
# `scipy.stats.morestats.WilcoxonResult` will be removed in SciPy 1.13.0, and the
# `scipy.stats.morestats` namespace will be removed in SciPy 2.0.0.

With this timeline established, we can remove private attributes like WilcoxonResult from the __all__ of these modules in 1.13.0.

The logic for public attributes is unchanged, but the deprecation message is more specific about when the module will be removed.

from scipy.stats.morestats import wilcoxon
# DeprecationWarning: Please import `wilcoxon` from the `scipy.stats` namespace; 
# the `scipy.stats.morestats` namespace is deprecated and will be removed in SciPy 2.0.0.

It sounds like there was a decision to remove these namespaces in SciPy 2.0.0, so import scipy.stats.morestats will work until 2.0.0, but was there a decision that that from scipy.stats.morestats import wilcoxon has to work until then? We could adjust the message to state that scipy.stats.morestats.wilcoxon will be removed in 1.13.0 and the whole namespace will be removed in 2.0.0.


  1. When getattring something from one of these modules that does not exist, we get an erroneous error message.
import scipy.stats.morestats
getattr(scipy.stats.morestats, 'ekki')
# AttributeError: scipy.stats.morestats is deprecated and has no attribute ekki. 
# Try looking in scipy.stats instead.

Strings that result in this message are never present in the subpackage namespace, as far as I can tell, so there is no reason to suggest looking there. Now:

import scipy.stats.morestats
getattr(scipy.stats.morestats, 'ekki')
# AttributeError: `scipy.stats.morestats` has no attribute `ekki`; furthermore,
# `scipy.stats.morestats` is deprecated and will be removed in SciPy 2.0.0.

Additional information

The logic here is easily abstracted to work for any module/subpackage by specifying the appropriate strings. The implementation could live in _lib so we can avoid copy-paste and have a consistent message format throughout SciPy. That is work for a follow-up.

@mdhaber mdhaber added scipy.stats deprecated Items related to behavior that has been deprecated labels Jun 9, 2023
@mdhaber
Copy link
Contributor Author

mdhaber commented Jun 9, 2023

Test passed locally before, then CI merged main and they fail. Looks like this is because atleast_1d is no longer imported in scipy.stats._morestats. Without a test like the one introduced in this PR, attributes may be removed accidentally during the deprecation period. Shall I add atleast_1d back to _morestats.py to make the test pass, or should it be removed from __all__ of morestats.py?

@mdhaber
Copy link
Contributor Author

mdhaber commented Jul 1, 2023

@h-vetinari @j-bowhay thought this might be up your alley. If not of interest, no problem; I'll go ahead and close.

Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

Thanks @mdhaber will give this proper look soon

scipy/stats/morestats.py Show resolved Hide resolved
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Thanks for the ping @mdhaber!

The plan sounds sensible to me. I haven't gone over it with a fine-toothed comb, but it looks good to me.

scipy/stats/morestats.py Show resolved Hide resolved
scipy/stats/morestats.py Show resolved Hide resolved
@h-vetinari
Copy link
Member

Without a test like the one introduced in this PR, attributes may be removed accidentally during the deprecation period. Shall I add atleast_1d back to _morestats.py to make the test pass, or should it be removed from __all__ of morestats.py?

Sorry, didn't notice this aspect before. I'd say add it back for now, but no strong preference.

@mdhaber
Copy link
Contributor Author

mdhaber commented Jul 3, 2023

Thanks @j-bowhay @h-vetinari. I added atleast_1d back to _morestats.py to fix the failure, and the remaining failure is unrelated. (I'll fix that in another PR shortly.)

Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

Thanks @mdhaber this looks like a good improvement to me. Happy to add the follow-up to my to-do list unless you already have it in the works

@j-bowhay j-bowhay added this to the 1.12.0 milestone Jul 4, 2023
@j-bowhay j-bowhay merged commit 7ee18da into scipy:main Jul 4, 2023
23 of 24 checks passed
@mdhaber
Copy link
Contributor Author

mdhaber commented Jul 4, 2023

I don't. It would be great if you'd like to follow up with the generic version.

alugowski pushed a commit to alugowski/scipy that referenced this pull request Jul 16, 2023
* DEP: scipy.stats.morestats: improve deprecation warnings

* DEP: scipy.stats.morestats: refactor deprecation warnings

* Apply suggestions from code review

* MAINT: stats.morestats: import atleast1D
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.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants