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

Restrict the T attribute to two-dimensional arrays #245

Merged
merged 4 commits into from
Sep 13, 2021
Merged

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Aug 30, 2021

This PR:

  • restricts the T attribute to two-dimensional arrays in agreement with the mathematical definition of the transpose. For array instances which are not two-dimensional, the specification recommends an error be thrown.
  • includes a note explaining the rationale for limiting to two-dimensions and adds a recommendation to use the permute API (note: the permute specification will be added in a subsequent PR; see gh-247).
  • removes unnecessary blank lines found in various notes.

Background

Discussion as to why the current behavior of T attribute is problematic can be found in gh-228.

By limiting the attribute to two-dimensional arrays, we retain the current behavior for .T for the primary use case of taking the transpose of a matrix, while removing the less desired behavior of reversing all axes.

@kgryte kgryte added the API change Changes to existing functions or objects in the API. label Aug 30, 2021
@rgommers
Copy link
Member

The array instance must be two-dimensional.

(making the same comment twice today, xref pytorch/pytorch#64180): gh-228 seemed to say to raise an error for >2-D only. Cc @lezcano

@kgryte
Copy link
Contributor Author

kgryte commented Aug 30, 2021

Yeah, I am a bit conflicted here. Yes, taking the transpose of a 1D array could be a no-op, but, as discussed in numpy/numpy#7495, it is questionable whether this is actually what you want. In which case, opted to be more restrictive in limiting to only 2D arrays.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 13, 2021

Based on discussions in the consortium meeting held 09-02-2021, no objections were made to limiting to 2-dimensional arrays.

While consensus determined that the transpose of a 1-dimensional array is ambiguous (does a user expect a no-op or actually want to convert a row vector to a column vector or vice versa?), less ambiguous is the behavior of taking the transpose of a 0-dimensional array (a no-op).

However, the stance of this PR is that a user should be explicit in taking transpose of an empty two-dimensional array, rather than using a zero-dimensional shorthand, as not clear to a reader whether an author actually intended for a no-op or this is a bug. Accordingly, strictly limiting to 2D and not allowing 0D avoids this runtime ambiguity.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 13, 2021

Merging based on discussions during the 09-02-2021 consortium meeting.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants