-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
MAINT: stats: make reducing functions emit consistent warning when sample is too small or empty #20694
Conversation
@h-vetinari @rgommers @ilayn As participants in gh-19805, if you could suggest answers to these two questions and compare the before/after behavior, I'd appreciate it.
This PR is going to accumulate merge conflicts, and it would be nice to get it into the release candidate to get wider feedback on the elimination of the ad hoc warnings/errors and new standardized warnings. |
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.
Generally this looks really nice! :)
I changed each of these tests to look for the new, standard behavior instead.
Love this!
That said, should I just eliminate these function-specific tests? I need to add a generic test that looks for the new behavior anyway, and that would make these scattered tests redundant.
Fine by me to just do a generic test (modulo caveats below)
Should it check every function, or should I just check, say, a few representative functions
Parametrization is easy and if the tests run fast, I don't see a problem to run this for (almost) every function.
Agree with @h-vetinari's answers to the two questions - choices LGTM. |
[skip cirrus] [skip circle]
[skip circle] [skip cirrus]
[skip cirrus] [skip circle]
[skip circle] [skip cirrus]
OK, I think this is now doing what I intended to do here. A few limitations:
One remaining question: by design, this is going to produce some warnings where there were none before. Should we make the |
@tylerjereddy This is ready, but I went ahead and re-milestoned. Don't want the new warnings to cause grief in the release process. |
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.
I think this is a clear improvement and it would be a pity to miss the release (much less you having to keep fixing conflicts all the time). I'd vote for including this in 1.14
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.
Re-review just to make this thread more visible
I clicked on the link to the outdated commit and commented there. In short, that's all set. I wouldn't oppose a squash merge if you'll help field complaints about the new warnings (although I think we're removing many more than we're adding, and the ones we're adding are informative, so maybe this won't be necessary). Also, I think it was a good idea to add the function-specific information to the warning message when "small sample" is an unusual number, like 7 (as opposed to the more obvious 0-2). I can work on that as an immediate follow-up, though. Up to you @h-vetinari. In any case, thanks for the review. |
Bombs away! |
Could you please write a release-note, and make sure it shows up in #20784? |
Sure; please commit https://github.com/scipy/scipy/pull/20784/files#r1616624810 if it looks good to you. |
Thank you! I'll leave the committing to Tyler. :) |
Reference issue
Address stats part of gh-19805
What does this implement/fix?
Eliminates inconsistent errors and excessive warnings emitted by most stats reducing functions when 1D input is too small. Documents required sample sizes. Emits a single warning when NaN outputs are generated without NaNs in the input or when
nan_policy='omit'
.Return values (usually NaNs) are already correct for the most part; this just deals with warnings and errors.
Examples below are for
skew
, but the behavior of most reducing functions is adjusted.Before:
After: (maybe I should change "is too small" → "contains too few observations")
Additional information
Consider ignoring whitepace changes and reviewing the commits separately. Each has a distinct purpose summarized by the commit message.
@rgommers @h-vetinari all I'd ask for now (if you're still interested in gh-19805) is to 1) do some before/after testing to see whether you prefer the new behavior (almost all reducing functions are fair game) and 2) help me answer these two questions.
Questions before I continue:
wilcoxon
with one argument,mannwhitneyu
with two arguments, andkruskal
with three arguments)?To do:
mode
,f_oneway
)_axis_nan_policy
test - it's very broken because it actually looked for the warnings/errors produced by each functionFor a follow-up - fix for array-API.
We still need to adjust a lot of things about the documentation. Some documentation assumes the samples are 1D, whereas in others it is careful to refer to axis-slices of ND arrays. I would prefer that we write the API documentation using 1D language but consistently link to a tutorial that describes how
axis
/nan_policy
/keepdims
work with N-D arrays. That's for a separate PR.Also, most hypothesis tests can easily get a
method
argument that computes a more accurate p-values instead of returning NaN for small samples (e.g. likepearsonr
andanderson_ksamp
now have). That can be a series of follow-up PRs.