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

ENH: stats.skewtest: add array-API support #20597

Merged
merged 10 commits into from
May 2, 2024
Merged

Conversation

j-bowhay
Copy link
Member

@j-bowhay j-bowhay commented Apr 28, 2024

Reference issue

Towards #20544

What does this implement/fix?

Adds array API support to stats.skewtest

Additional information

@github-actions github-actions bot added scipy.stats enhancement A new feature or improvement labels Apr 28, 2024
@j-bowhay j-bowhay requested a review from mdhaber April 28, 2024 06:45
scipy/stats/_stats_py.py Outdated Show resolved Hide resolved
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.

On my phone this looks really good! I'll take a closer look (including at your where question) when I'm at a computer.

scipy/stats/_stats_py.py Outdated Show resolved Hide resolved
scipy/stats/_stats_py.py Show resolved Hide resolved
@mdhaber mdhaber mentioned this pull request Apr 28, 2024
74 tasks
scipy/_lib/_array_api.py Outdated Show resolved Hide resolved
@mdhaber
Copy link
Contributor

mdhaber commented Apr 30, 2024

scipy/stats/tests/test_stats.py Outdated Show resolved Hide resolved
scipy/stats/tests/test_stats.py Outdated Show resolved Hide resolved
scipy/stats/tests/test_stats.py Outdated Show resolved Hide resolved
@j-bowhay
Copy link
Member Author

j-bowhay commented Apr 30, 2024

Re: CI failure, see https://github.com/scipy/scipy/pull/20595/files#r1582010187

I am not sure I quite follow, I am not passing in xp to xp_assert_close?

EDIT: ah wait I see 🤦

Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

diff LGTM apart from the unresolved comment about nan_policy! I haven't checked thoroughly whether anything else is needed.

scipy/stats/_stats_py.py Show resolved Hide resolved
@j-bowhay
Copy link
Member Author

Ok CI is green, hopefully that is all comments addressed

@lucascolley lucascolley added the array types Items related to array API support and input array validation (see gh-18286) label Apr 30, 2024
@lucascolley
Copy link
Member

feel free to merge @mdhaber

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.

I think this is fine except for question of default rtol of xp_assert_close. What do you think of it based on what it does for this PR and gh-20595? I think other maintainers need to be aware and on board before we pull the trigger. Are either of you willing to champion this, maybe submitting it as a separate PR? I have another RFC I'll be attending to this week (new distribution infrastructure).

But @j-bowhay please feel free to start on another conversion in the meantime, if you'd like!

Comment on lines 294 to 296
floating = xp.isdtype(desired.dtype, ('real floating', 'complex floating'))
if rtol is None and floating:
rtol = xp.finfo(desired.dtype).eps**0.5 * 4
Copy link
Contributor

@mdhaber mdhaber May 1, 2024

Choose a reason for hiding this comment

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

I didn't think about this carefully before, and I had it like this. But I started wondering whether it should be:

Suggested change
floating = xp.isdtype(desired.dtype, ('real floating', 'complex floating'))
if rtol is None and floating:
rtol = xp.finfo(desired.dtype).eps**0.5 * 4
floating = xp.isdtype(actual.dtype, ('real floating', 'complex floating'))
if rtol is None and floating:
rtol = xp.finfo(actual.dtype).eps**0.5 * 4

If the developer decides to turn off the dtype check, what do they expect to happen when the dtypes are different / what would be most useful?

Suppose rtol is based on the reference dtype (as it is now).

  • If we are comparing a low-precision actual value against a high-precision reference, that is too hard of a test to pass.
  • If we are comparing a high-precision actual value against a low-precision reference, that is too easy of a test to pass.

So perhaps this should be the other way around - the actual value should be compared against the reference value according to the actual value's own precision.

@mdhaber
Copy link
Contributor

mdhaber commented May 2, 2024

Merged main. If tests still pass, I'll go ahead and merge.

@mdhaber mdhaber merged commit 75fb00d into scipy:main May 2, 2024
30 checks passed
@mdhaber mdhaber modified the milestones: 1.15.0, 1.14.0 May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
array types Items related to array API support and input array validation (see gh-18286) enhancement A new feature or improvement scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants