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.

Sign up for GitHub

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

Jump to bottom

TST: stats.ttest_1samp: fix xslow test #20642

Merged
merged 2 commits into from May 5, 2024
Merged

TST: stats.ttest_1samp: fix xslow test #20642

merged 2 commits into from May 5, 2024

Conversation

mdhaber
Copy link
Contributor

@mdhaber mdhaber commented May 4, 2024

Reference issue

Closes gh-20641

What does this implement/fix?

Fixes the one test failure that occurs twice with each backend tested.

Additional information

[gh20641 aa257e0deb] TST: stats.ttest_1samp: fix xslow test
 1 file changed, 3 insertions(+), 3 deletions(-)
(scipy-dev) matthaberland@Matts-MacBook-Air scipy % git status
On branch gh20641
nothing to commit, working tree clean
(scipy-dev) matthaberland@Matts-MacBook-Air scipy % pytest scipy/stats/tests/test_stats.py::TestStudentTest::test_pvalue_ci -v
========================================= test session starts =========================================
platform darwin -- Python 3.11.6, pytest-7.4.3, pluggy-1.5.0 -- /Users/matthaberland/miniforge3/envs/scipy-dev/bin/python3.11
cachedir: .pytest_cache
hypothesis profile 'deterministic' -> database=None, deadline=None, print_blob=True, derandomize=True, suppress_health_check=[HealthCheck.data_too_large, HealthCheck.filter_too_much, HealthCheck.too_slow, HealthCheck.large_base_example, HealthCheck.function_scoped_fixture, HealthCheck.differing_executors]
rootdir: /Users/matthaberland/Desktop/scipy
configfile: pytest.ini
plugins: fail-slow-0.5.0, hypothesis-6.88.1, anyio-4.3.0, cov-4.1.0, skip-slow-0.0.5, xdist-3.3.1
collected 6 items                                                                                     

scipy/stats/tests/test_stats.py::TestStudentTest::test_pvalue_ci[less-numpy] PASSED             [ 16%]
scipy/stats/tests/test_stats.py::TestStudentTest::test_pvalue_ci[less-array_api_strict] PASSED  [ 33%]
scipy/stats/tests/test_stats.py::TestStudentTest::test_pvalue_ci[less-torch] PASSED             [ 50%]
scipy/stats/tests/test_stats.py::TestStudentTest::test_pvalue_ci[greater-numpy] PASSED          [ 66%]
scipy/stats/tests/test_stats.py::TestStudentTest::test_pvalue_ci[greater-array_api_strict] PASSED [ 83%]
scipy/stats/tests/test_stats.py::TestStudentTest::test_pvalue_ci[greater-torch] PASSED          [100%]

========================================== 6 passed in 8.73s ==========================================

@mdhaber mdhaber added scipy.stats maintenance Items related to regular maintenance tasks labels May 4, 2024
mdhaber
mdhaber commented May 4, 2024
View reviewed changes
scipy/stats/tests/test_stats.py Outdated
Comment on lines 3714 to 3715
data = data.astype(np.float64, copy=True) # ensure byte order
data = xp.asarray(data, dtype=xp.float64)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
data = data.astype(np.float64, copy=True) # ensure byte order
data = xp.asarray(data, dtype=xp.float64)
data = data.astype(np.float64, copy=True) # ensure byte order
# `float64` is needed here because of the range of `alpha`
data = xp.asarray(data, dtype=xp.float64)

Copy link
Contributor

Choose a reason for hiding this comment

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

I did check that all these incantations pass locally on x86_64 Linux, while they fail on main:

SCIPY_XSLOW=1 SCIPY_DEVICE=cuda python dev.py test -j 32 -b all -- -k "test_pvalue_ci"
SCIPY_XSLOW=1 python dev.py test -j 32 -b all -- -k "test_pvalue_ci"
SCIPY_XSLOW=1 python dev.py test -j 32 -- -k "test_pvalue_ci"

I tried to play picky reviewer and force float32 instead of float64 locally, and saw overflow/invalid value warnings, so that seems like a plausible justification for non-stats expert at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

(obivously the first one has limited value, since it is cpu only)

@WarrenWeckesser
Copy link
Member

WarrenWeckesser commented May 4, 2024 •
edited

@lucascolley, I've only been following the array API project peripherally, so I don't think I'll dig into it now to review this.

I hope all reviewers of PRs that touch XSLOW tests will be sure to run the tests themselves to ensure all is working as expected before merging. Alternatively (or additionally), perhaps the CI gurus could add a CI job that runs the XSLOW tests when a flag such as [run xslow] is provided in a commit message.

@lucascolley lucascolley removed the request for review from WarrenWeckesser May 4, 2024 18:52
@tylerjereddy tylerjereddy added this to the 1.14.0 milestone May 4, 2024
lucascolley
lucascolley reviewed May 5, 2024
View reviewed changes
scipy/stats/tests/test_stats.py Outdated
Comment on lines 3714 to 3715
data = data.astype(np.float64, copy=True) # ensure byte order
data = xp.asarray(data, dtype=xp.float64)
Copy link
Member

Choose a reason for hiding this comment

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

Am I missing something with the "ensure byte order" comment? Can we not just include the copy in asarray?

Suggested change
data = data.astype(np.float64, copy=True) # ensure byte order
data = xp.asarray(data, dtype=xp.float64)
# `float64` is needed here because of the range of `alpha`
data = xp.asarray(data, dtype=xp.float64, copy=True)

Copy link
Contributor Author

@mdhaber mdhaber May 5, 2024 •
edited

Choose a reason for hiding this comment

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

hypothesis generates them in some nonstandard format that torch.asarray won't accept. Might be possible to make it generate more standard float64s.

Copy link
Member

Choose a reason for hiding this comment

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

okay, if that isn't possible then I think this looks okay. Maybe worth adding a comment as to why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted the test strategy to generate normal float64s. I think that the old strategy must have been left over from before the test took its final form.

@tylerjereddy tylerjereddy merged commit 7f0a5cb into scipy:main May 5, 2024
@tylerjereddy
Copy link
Contributor

I tested locally again and things were still "good" so I merged it. I think the comments were addressed for the small patch here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerjereddy tylerjereddy tylerjereddy left review comments

@lucascolley lucascolley lucascolley left review comments

@j-bowhay j-bowhay Awaiting requested review from j-bowhay

Assignees
No one assigned
Labels
maintenance Items related to regular maintenance tasks scipy.stats
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.

BUG: stats: Two new XSLOW test failures
4 participants
@mdhaber @WarrenWeckesser @tylerjereddy @lucascolley