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

BUG: stats.ttest_1samp: fix use of keepdims #20734

Merged
merged 4 commits into from
May 31, 2024

Conversation

sdiebolt
Copy link
Contributor

@sdiebolt sdiebolt commented May 17, 2024

Reference issue

Partially addresses gh-20725.

What does this implement/fix?

This PR fixes an error raised when using the keepdims argument of scipy.stats.ttest_1samp. The error comes from the _axis_nan_policy_factory decorator that tries to expand the dimensions of all returns from decorator functions. Some of the decorated functions return simple integers that cannot be expanded, thus the error:

def _add_reduced_axes(res, reduced_axes, keepdims):
"""
Add reduced axes back to all the arrays in the result object
if keepdims = True.
"""
return ([np.expand_dims(output, reduced_axes) for output in res]
if keepdims else res)

Additional information

test_keepdims originally raised an error when adding stats.ttest_1samp due to a division-by-zero warning. The error is due to some test cases where omitting NaNs leads to samples of size 1, thus the division-by-zero when computing the SEM. To fix this, I set the too_small argument to a callable that checks if the sample to test has at least size 2. I could not simply use too_small=1 however, since it seems popmean is considered a sample (ie., n_samples is set to 2 in the decorator of ttest_1samp).

Adding all other functions from axis_nan_policy_cases raises many more errors that I'll try to check now.

Also set the `too_small` argument of `ttest_1samp`'s decorator to a
callable that checks if the `a` argument has at least 1 sample.
Otherwise, we get a division-by-zero warning.
@github-actions github-actions bot added scipy.stats defect A clear bug or issue that prevents SciPy from being installed or used as expected labels May 17, 2024
@lucascolley lucascolley requested a review from mdhaber May 17, 2024 09:35
@sdiebolt
Copy link
Contributor Author

@mdhaber Ok so it seems most errors that occur when running test_keepdims on all axis_nan_policy_cases are due to small sample sizes (occurring for a number of reasons, from samples becoming too small after omitting NaNs, to samples being simply too small or having a degenerate dimension, e.g. with the case sample_shape=(20, 0)). What would you recommend? Should I try to modify test_keepdims parameters in this PR to fit axis_nan_policy_cases?

@mdhaber
Copy link
Contributor

mdhaber commented May 17, 2024

In that case, maybe leave the existing test alone if it's that strict. Instead, add a new test like it that doesn't generate such cases, since they seem orthogonal to the issue of non-expandable scalars. The inconsistent behavior w.r.t. small samples is being fixed separately; when that's resolved, we can probably combine the tests more easily.

@lucascolley lucascolley changed the title BUG: fix use of keepdims for scipy.stats.ttest_1samp BUG: stats.ttest_1samp: fix use of keepdims May 17, 2024
@mdhaber mdhaber added this to the 1.14.0 milestone May 30, 2024
@mdhaber mdhaber added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 30, 2024
@mdhaber
Copy link
Contributor

mdhaber commented May 30, 2024

This fixes a bug as-is, so I'd like this to make it into 1.14.0.
We just did a major cleanup of warnings/errors for functions with sample sizes <= too_small. We need to do a follow-up of functions that need a different too_small than they have, so I don't want that to hold up or be touched in this issue.
I'll leave gh-20725 open after this so we remember to add other functions to test_keepdims after fixing too_small.

Copy link
Contributor

@mdhaber mdhaber left a comment

Choose a reason for hiding this comment

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

If tests pass, I'll merge.
@tylerjereddy can we backport this fix into the next release candidate?

@mdhaber mdhaber marked this pull request as ready for review May 30, 2024 21:01
@mdhaber mdhaber merged commit 1ead6a9 into scipy:main May 31, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-candidate This fix should be ported by a maintainer to previous SciPy versions. defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants