-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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.skew: add array-API support #20541
Conversation
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.
LGTM, I had a look at the individual commits as suggested and it seems right to me. Also the changes on the tests look good.
I was just wondering if we might want to move the first if
logic into the array API check function. But that's another topic anyway.
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.
agreed, beat me to it @tupui :) just a small typo and one thought about the testing
@@ -1121,6 +1121,8 @@ def _var(x, axis=0, ddof=0, mean=None): | |||
@_axis_nan_policy_factory( | |||
lambda x: x, result_to_tuple=lambda x: (x,), n_outputs=1 | |||
) | |||
# nan_policy handled by `_axis_nan_policy, but needs to be left |
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.
# nan_policy handled by `_axis_nan_policy, but needs to be left | |
# nan_policy handled by `_axis_nan_policy`, but needs to be left |
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.
Oops I'll fix this next time.
def test_empty_1d(self): | ||
# This is not essential behavior to maintain w/ array API |
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.
perhaps more clear to @skip_xp_backends(np_only=True)
, although I don't have a strong opinion. Right now, not parameterising with xp
very clearly means 'this is just for NumPy'. In the future, when we potentially have the majority of tests converted, that might not be so clear. But happy with this for now, can be left for the future.
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 will consider tacking this on to my next PR. But honestly this test can probably just be removed. We've talked about silencing this warning throughout stats.
Reference issue
Follow-up to gh-20292
What does this implement/fix?
This adds array-API support to
scipy.stats.skew
. It might be useful to review one commit at a time:nan_policy
support is handled by the_axis_nan_policy
decorator)_axis_nan_policy
decorator raise when it gets arguments that won't currently work with the array API. It only works when these parameters are passed as keyword arguments (which they should be anyway), but I am not too concerned because a) array-API support is still experimental and behind an environment variable and b) these features should be available with non-NumPy backends by the time the environment variable is removed, at which point this error will be removed. Nothing like this commit will be needed with other array-API conversions.Additional information
After this, I will open an issue with a list of stats functions I think are ripe for array API support. We can point to this PR as an example and hopefully crowdsource the effort. I'd be willing to review PRs for functions on that list.