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: stats: deprecate function-specific versions of warnings #16266

Closed
wants to merge 4 commits into from

Conversation

mdhaber
Copy link
Contributor

@mdhaber mdhaber commented May 24, 2022

Reference issue

Followup to gh-15923

What does this implement/fix?

gh-15923 consolidated stats warnings into a few superclasses. It was suggested to simplify further by removing the function-specific warnings. This begins the process of removing the function-specific warnings by deprecating them.

Additional information

After merge, this needs an entry in the release notes about the deprecation. Also, the warnings should be removed in 1.11.0, so something should be done as a reminder. (Personally, I think a note in gh-15765 is all that is needed, rather than a separate issue for every deprecation.)

@mdhaber mdhaber added scipy.stats maintenance Items related to regular maintenance tasks labels May 24, 2022
@mdhaber mdhaber requested review from rgommers and ev-br May 24, 2022 23:15
@mdhaber mdhaber mentioned this pull request May 24, 2022
29 tasks
@h-vetinari
Copy link
Member

Personally, I think a note in gh-15765 is all that is needed, rather than a separate issue for every deprecation.

I understand the sentiment - raising a separate issue can appear noisy - but I think the results of #15765 do make a case for it IMO:

  • the respective tasks are much more discoverable (e.g. "good first issue" labels or being milestoned for a release)
  • there's a dedicated place to discuss the deprecation at hand without spamming a meta issue (e.g. delay, accelerate, abandon)
  • several new contributors have joined through tackling one of those "tiny" issues

@j-bowhay
Copy link
Member

I understand the sentiment - raising a separate issue can appear noisy - but I think the results of #15765 do make a case for it IMO:

I agree and would add the part of the reason it has been so noisy is that so many previous deprecations have slipped through the net which means there has been a lot to catch up on. I think once everything has been tided it the amount of noise will decrease significantly.

@rgommers
Copy link
Member

I understand the sentiment - raising a separate issue can appear noisy - but I think the results of #15765 do make a case for it IMO:

I agree and would add the part of the reason it has been so noisy is that so many previous deprecations have slipped through the net which means there has been a lot to catch up on. I think once everything has been tided it the amount of noise will decrease significantly.

As discussed in the community meeting today: let's ensure to have a single issue per future release going forward, with the appropriate details of what must be done. Having many issues like we had for tackling the deprecations in 1.9.0 is not a problem; what is a problem is having many issues that aren't actually actionable. The single issue that is actionable next year can then be expanded into multiple when it becomes actionable (if desired).

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 @mdhaber. This doesn't look quite right to me: the deprecation warning won't actually be triggered by the use of stats.F_onewayBadInputSizesWarning or other classes.

The docstring edits don't appear it looks like, but I'd prefer not to have them there - nothing in f_oneway itself is deprecated.

I believe a better way of doing this is to add a __getattr__ in stats/__init__.py, and emitting a warning when using a class. And no docs changes other than in the release notes.

@mdhaber
Copy link
Contributor Author

mdhaber commented May 25, 2022

emitting a warning when using a class

i.e. in the __init__ method of the class? This makes sense, and I should have done that instead of emitting the warning in the statement before/after the object is instantiated.

add a __getattr__ in stats/init.py

What is this for? Would this raise upon import? That doesn't sound desirable. I don't think we do that for any other functions/classes that we deprecate.

won't actually be triggered by the use of stats.F_onewayBadInputSizesWarning

True. I didn't imagine that users are instantiating these classes themselves; I covered the case in which they except them. But there is no harm in warning in the __init__ method of the class, and it would be more concise. I should have done that.

The docstring edits don't appear it looks like

Weird.

but I'd prefer not to have them there - nothing in f_oneway itself is deprecated.

I agree that nothing in f_oneway is being deprecated, but something is being deprecated, and that is affecting the behavior of f_oneway. So it seemed reasonable to me that the documentation should mention it. Also, I thought we agreed on it here.
image

That said, I don't enjoy fighting with Sphinx, so if you don't think it's needed, I'll remove it.

Update: done.

@mdhaber mdhaber requested a review from rgommers May 25, 2022 22:21
@rgommers
Copy link
Member

rgommers commented May 26, 2022

i.e. in the __init__ method of the class? This makes sense, and I should have done that instead of emitting the warning in the statement before/after the object is instantiated.

No, because the way you use these warning objects means you never instantiate them (see for example the test cases you touch here). I really mean "if a user types stats.F_onewayBadInputSizesWarning". Otherwise I don't see the point and you may as well remove them without a deprecation.

add a __getattr__ in stats/init.py

What is this for? Would this raise upon import? That doesn't sound desirable. I don't think we do that for any other functions/classes that we deprecate.

Just have a look at the many def __getattr__(name): in the code base. E.g.:

def __getattr__(name):
    if name == 'F_onewayBadInputSizesWarning':
        warnings.warn("F_onewayBadInputSizesWarning is deprecated, please use ...."
                      category=DeprecationWarning, stacklevel=2)

@mdhaber
Copy link
Contributor Author

mdhaber commented May 26, 2022

Where does this __getattr__ go?

image

Really in a new file scipy/stats/init.py? If you meant scipy/stats/__init__.py like you wrote above, I don't think that works.

And if so, what should be the last line of __getattr__ (i.e. what does it return)?

@rgommers
Copy link
Member

Really in a new file scipy/stats/init.py? If you meant scipy/stats/__init__.py like you wrote above, I don't think that works.

I did write scipy/stats/__init__.py in #16266 (review)

And if so, what should be the last line of __getattr__ (i.e. what does it return)?

return __dict__[name] does the job it looks like - however, it messes with tab completion. Which is not great.

So I think just rip it out then. There's no good way of doing this as far as I can tell without adopting lazy imports (as in https://scientific-python.org/specs/spec-0001/) fully.

@mdhaber
Copy link
Contributor Author

mdhaber commented May 26, 2022

Weird. I tried adding:

def __getattr__(name):
    if name == "F_onewayConstantInputWarning":
        warnings.warn("Deprecated", DeprecationWarning)
    return __dict__[name]

to scipy/stats/__init__.py, and it does not get called when I import scipy.stats and evaluate scipy.stats.F_onewayConstantInputWarning or when I from scipy.stats import F_onewayConstantInputWarning. @tupui is my witness!
I will open a new PR that rips them out.

@mdhaber mdhaber closed this May 26, 2022
@mdhaber
Copy link
Contributor Author

mdhaber commented May 26, 2022

Ah, and I see that GH turns __init.py__ to init.py when it is copy-pasted (#16266 (comment)).

The funny thing is that it actually replaced your double-underscores with asterisks, so I thought this was an intentional correction.

image

@rgommers
Copy link
Member

The ... > Quote reply preserves Markdown formatting, so I try to always use that.

@mdhaber
Copy link
Contributor Author

mdhaber commented May 26, 2022

I see; must have forgotten this once. No problem!

#16289 is the new attempt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants