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

Add expand_dims #8407

Merged
merged 19 commits into from
Dec 1, 2023
Merged

Add expand_dims #8407

merged 19 commits into from
Dec 1, 2023

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Nov 3, 2023

This allows adding a dimension of an array. Which is relevant when for example using keepdims in NamedArray.reduce.

Normally you can do this with None but that's not really allowed in the getitem documentation currently https://data-apis.org/array-api/2022.12/API_specification/generated/array_api.array.__getitem__.html

Should follow the standard but with an additional dim argument, dim should be preferred over the axis argument.
https://data-apis.org/array-api/draft/API_specification/generated/array_api.expand_dims.html

xref #8344, #8406

@github-actions github-actions bot added the topic-NamedArray Lightweight version of Variable label Nov 3, 2023
@Illviljan Illviljan marked this pull request as ready for review November 3, 2023 16:22
@dcherian
Copy link
Contributor

dcherian commented Nov 3, 2023

Should follow the standard but with an additional dim argument, dim should be preferred over the axis argument.

Thanks @Illviljan we've been leaning toward not providing Array API specifically because there's quite some potential for confusing mismatches like this one.

@Illviljan
Copy link
Contributor Author

Illviljan commented Nov 3, 2023

What mismatch? This is only expanding on the standard which in my view should be fine.

How do you plan on handling expanding dims (arrayapi[None]) without this function?

We have to handle axis in all our methods, I don't see why we can't use those helper functions in the array_api namespace as well.
I don't mind our methods (NamedArray.mean()) being more restrictive though.

The current _array_api is not public yet either, at this point it's just solving issues that we aren't yet testing for since we barely test any arrayapi compliant arrays at the moment.

@andersy005
Copy link
Member

andersy005 commented Nov 9, 2023

ks @Illviljan we've been leaning toward not providing Array API specifically because there's quite some potential for confusing mismatches like this one.

after reading data-apis/array-api#698 (comment), i support the idea of implementing the same (or expanded) syntax and semantics as the array API standard for a subset of the array API spec when possible.

it appears that as long as we don't add a __array_namespace__ method to xarray.NamedArray to declare array API compatibility, trying to align with the array API would be worth of our time and effort.

to proceed in the right direction, i'm curious to hear what your take is, @dcherian / @Illviljan

@andersy005
Copy link
Member

@Illviljan, since we do not plan on declaring xarray.NamedArray as an array API compliant/compatible object, would it be possible to directly implement these methods on NamedArray instead of going through the private module?

@Illviljan
Copy link
Contributor Author

Considering how much xarray has pushed upstream array-libraries to follow various standards, ufunc/array_function/array_api, I think it's bad form to not make an effort to respect the standards ourselves. Whether we start supporting it officially in the future or not.

I wouldn't mind adding NamedArray.expand_dims that just calls xp.expand_dims afterwards though, that would just be an extension for array api users rather than a weird quirk of NamedArray.

Copy link
Member

@andersy005 andersy005 left a comment

Choose a reason for hiding this comment

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

thank you, @Illviljan

@andersy005 andersy005 merged commit 4550a01 into pydata:main Dec 1, 2023
28 checks passed
@Illviljan Illviljan deleted the add_expand_dims branch December 12, 2023 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-NamedArray Lightweight version of Variable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants