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 specification for moving array axes to new positions #656

Merged
merged 5 commits into from Sep 19, 2023

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Jul 13, 2023

This PR

  • resolves Adding moveaxis & swapaxes #483 by adding a specification for moveaxis.
  • signatures across array libraries are consistent. Only TensorFlow does not have moveaxis in its main namespace, but it does include it in its experimental NumPy namespace.

Questions

  • Bikeshed: we intentionally ensure consistent naming with permute_dims and expand_dims. Should we be consistent here in renaming moveaxis to, e.g., move_dims? PyTorch predominantly uses the dims convention and aliases moveaxis as movedim, which is not a perfect match given the missing _ and plural form dims.
    • Update: leaving name as is as moveaxis given widespread array library usage of moveaxis. Renaming to something consistent with permute_dims and expand_dims would likely cause unnecessary churn for insufficient benefit.

@kgryte kgryte added the API extension Adds new functions or objects to the API. label Jul 13, 2023
@kgryte kgryte added this to the v2023 milestone Jul 13, 2023
@rgommers
Copy link
Member

This looks fairly straightforward, nice.

Bikeshed: we intentionally ensure consistent naming with permute_dims and expand_dims. Should we be consistent here in renaming moveaxis to, e.g., move_dims?

Given signatures for all libraries match and the moveaxis name is fine, I think this is about as good an example as we can have for "go with prior art".

@kgryte
Copy link
Contributor Author

kgryte commented Jul 27, 2023

During the consortium workgroup meeting on 27 July 2023, consensus was to move this API forward and that moveaxis is the preferred name given its widespread usage in the array library ecosystem.

Accordingly, this PR should be ready for merge.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 19, 2023

As this PR is straightforward, the API has broad support across the ecosystem, and there is working group consensus to move this API forward, I'll go ahead and merge. Any further updates prior to 2023 spec finalization can be made in subsequent PRs.

@kgryte kgryte merged commit b823223 into data-apis:main Sep 19, 2023
3 checks passed
@kgryte kgryte deleted the moveaxis branch September 19, 2023 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API extension Adds new functions or objects to the API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding moveaxis & swapaxes
2 participants