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

TST: stats: refactor tests of normality tests #20756

Merged
merged 1 commit into from
May 21, 2024

Conversation

mdhaber
Copy link
Contributor

@mdhaber mdhaber commented May 20, 2024

Reference issue

Follow-up to gh-20597, gh-20715, gh-20736

What does this implement/fix?

As discussed in referenced issues, this PR refactors tests of stats.skewtest, stats.kurtosistest, and stats.normaltest to make them more maintainable.

@mdhaber mdhaber added scipy.stats maintenance Items related to regular maintenance tasks labels May 20, 2024

# nan_policy only compatible with NumPy arrays
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delete all these. nan_policy is implemented with axis_nan_policy decorator and tested with strong, property-based tests.

@@ -6423,26 +6376,6 @@ def test_axis(self, xp):
xp_assert_close(res.pvalue, resT.pvalue)


@array_api_compatible
def test_skewtest_too_few_samples(xp):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved these up into classes.

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 big improvement, one query from my end then this is good to merge

raise ValueError(
f"skewtest is not valid with less than 8 samples; {n} samples were given.")
message = ("`skewtest` requires at least 8 observations; "
f"{n} observations were given.")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe if you're coming near here in a future PR, for consistency with the kurtosistest

Suggested change
f"{n} observations were given.")
f"{n=} observations were given.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Yes actually I can do that in gh-20694, which will have merge conflicts with this one.

Comment on lines -6257 to -6268
# Test axis=None (equal to axis=0 for 1-D input)
res = stats.normaltest(x_xp, axis=None)
xp_assert_close(res.statistic, st_normal)
xp_assert_close(res.pvalue, pv_normal)

res = stats.skewtest(x_xp, axis=None)
xp_assert_close(res.statistic, st_skew)
xp_assert_close(res.pvalue, pv_skew)

res = stats.kurtosistest(x_xp, axis=None)
xp_assert_close(res.statistic, st_kurt)
xp_assert_close(res.pvalue, pv_kurt)
Copy link
Member

Choose a reason for hiding this comment

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

Assume this is tested elsewhere as I couldn't see these tests in the updated version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. axis=None is tested by generic, property-based tests in test_axis_nan_policy.py.

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

@j-bowhay j-bowhay merged commit c3ce012 into scipy:main May 21, 2024
31 checks passed
@j-bowhay j-bowhay added this to the 1.14.0 milestone May 21, 2024
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

2 participants