8000 ENH: `stats.tmean`: add array API support by mdhaber · Pull Request #20965 · scipy/scipy · GitHub
[go: up one dir, main page]
Skip to content

ENH: stats.tmean: add array API support #20965

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
Merged
merged 3 commits into from
Jun 19, 2024
Merged

ENH: stats.tmean: add array API support #20965

merged 3 commits into from
Jun 19, 2024

Conversation

mdhaber
Copy link
Contributor
@mdhaber mdhaber commented Jun 15, 2024 •
edited
Loading

Reference issue

Toward gh-20544

What does this implement/fix?

Adds array API support to tmean and paves the way for adding support to other trimmed statistics.

Along the way, I discovered that _lazywhere not always preserving dtype was one of several reasons that tmean wasn't preserving type, so I fixed that. I added associated tests to _lazywhere, which also led to me making the scalar check in xp_assert tests optional, since _lazywhere is (presumably) supposed to return 0-D arrays instead of scalars to follow the example of where.

Additional information

The diff would be smaller with this function if we were to just replace the use of nanmean with xp_mean with nan_policy='omit', but that wouldn't help us with any of the other functions. Might as well keep the approach consistent unless we also want to add an xp_var, xp_max, xp_min, etc...

Looks like the failure is just gh-20963.

@mdhaber mdhaber added scipy.stats maintenance Items related to regular maintenance tasks labels Jun 15, 2024
@mdhaber mdhaber changed the title ENH: stats.tmean: add array API support ENH: stats.tmean: add array API support Jun 15, 2024
@github-actions github-actions bot added the enhancement A new feature or improvement label Jun 15, 2024
@lucascolley lucascolley added the array types Items related to array API support and input array validation (see gh-18286) label Jun 16, 2024
j-bowhay
j-bowhay requested changes Jun 17, 2024
View reviewed changes
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.

Minor request then I think this will be in good shape

@j-bowhay
Copy link
Member

Does this also close #20959 since this works?

In [4]: a = np.array(42)

In [6]: xp_assert_equal(a, a, allow_0d=True)

In [7]: xp_assert_close(a, a, allow_0d=True)

@lucascolley
Copy link
Member
lucascolley commented Jun 18, 2024 •
edited
Loading

I think that issue needs a little more discussion if we are going to make the default allow_0d=False.

@j-bowhay
Copy link
Member

I think that issue needs a little more discussion if we are going to make the default allow_0d=False.

But I assume there is no reason why we can't merge as is and leave the issue open to discuss changing the default?

@lucascolley lucascolley mentioned this pull request Jun 18, 2024
Open
@lucascolley lucascolley removed the maintenance Items related to regular maintenance tasks label Jun 18, 2024
@mdhaber
Copy link
Contributor Author
mdhaber commented Jun 18, 2024 •
edited
Loading

Right. That was my intent. Also, this PR is already a bit unwieldy, so I didn't want to also change the hundreds of existing uses of these functions so the one used by _lazywhere could leave allow_0d unspecified : )

Consider showing support for numpy/numpy#24897. I would by happy to work toward returning 0d arrays consistently if NumPy did.

j-bowhay
j-bowhay approved these changes Jun 19, 2024
View reviewed changes
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 7f0b0a1 into scipy:main Jun 19, 2024
34 of 35 checks passed
@j-bowhay j-bowhay added this to the 1.15.0 milestone Jun 19, 2024
mdhaber
mdhaber commented Jun 19, 2024
View reviewed changes
scipy/_lib/_util.py
# `result_type` should/will handle mixed array/Python scalars
# (data-apis/array-api#805) but doesn't yet. So in the meantime, this fails
# for array-api-strict.
dtype = xp.result_type(temp1.dtype, fillvalue)
Copy link
Contributor Author
@mdhaber mdhaber Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@j-bowhay I am at a loss for why this did not cause CI failure for torch in this PR, but it is causing a failure now in gh-20971. Any guesses?

@mdhaber mdhaber mentioned this pull request Jun 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-bowhay j-bowhay j-bowhay approved these changes

Assignees
No one assigned
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
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mdhaber @j-bowhay @lucascolley
0