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

Rename (and move) transpose to permute_dims #247

Merged
merged 5 commits into from
Sep 20, 2021
Merged

Rename (and move) transpose to permute_dims #247

merged 5 commits into from
Sep 20, 2021

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Aug 30, 2021

This PR:

  • renames (and moves) the current transpose API to permute_dims (which is consistent with the *_dims naming convention (e.g., expand_dims)), as discussed in gh-228.
  • makes the axes argument a required argument (as discussed in the consortium meeting held 2021-09-16).
  • removes default behavior of reversing all axes (as discussed in the consortium meeting held 2021-09-16).

Background

The current behavior of transpose where all axes are reversed, even for arrays having more than two dimensions, is considered problematic due to its conflicting with the mathematical definition and not operating on batches of matrices.

By renaming the transpose function to permute_dims and moving the API to the set of manipulation functions, we both (1) provide a functional API which allows consumers to achieve equivalent functionality possible today (see gh-245), thus providing a migration path, and (2) avoid conflicting definitions.

This PR further helps pave the way for a dedicated matrix_transpose API for operating on stacks of matrices.

@kgryte kgryte added API change Changes to existing functions or objects in the API. topic: Linear Algebra Linear algebra. labels Aug 30, 2021
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM

@asmeurer
Copy link
Member

asmeurer commented Sep 2, 2021

This is a new function, right (not in any of the existing APIs under this name)? Is there any reason to keep the default argument for axes?

@kgryte
Copy link
Contributor Author

kgryte commented Sep 2, 2021

@asmeurer It is a new function insofar as it has a different name, but still retains the same default behavior as transpose. Otherwise, what other default do you propose?

@asmeurer
Copy link
Member

asmeurer commented Sep 2, 2021

I would propose making axes a required argument. I don't feel strongly about it, but reversing seems like an odd default. Unless there are legitimate use-cases for reversing (I was never clear that there were).

@kgryte kgryte changed the title Rename (and move) transpose to permute Rename (and move) transpose to permute_dims Sep 20, 2021
@kgryte
Copy link
Contributor Author

kgryte commented Sep 20, 2021

Updated the PR such that the function name is permute_dims, the axes argument is required, and the default behavior is no longer to reverse all axes. This was discussed and approved during the most recent consortium meeting held on 2021-09-16. As such, will go ahead and merge...

@kgryte kgryte merged commit d57a1f8 into main Sep 20, 2021
@kgryte kgryte deleted the rename-transpose branch September 20, 2021 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API. topic: Linear Algebra Linear algebra.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants