Skip to content

[Consistency] Default axes in linalg.diagonal and linalg.trace. #215

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

Closed
lezcano opened this issue Jul 1, 2021 · 5 comments
Closed

[Consistency] Default axes in linalg.diagonal and linalg.trace. #215

lezcano opened this issue Jul 1, 2021 · 5 comments
Labels
topic: Linear Algebra Linear algebra.

Comments

@lezcano
Copy link
Contributor

lezcano commented Jul 1, 2021

The functions linalg.trace and linalg.diagonal have as default axis1=0, axis2=1.

This goes against the rest of the documentation, in which a batch of matrices is defined as a tensor with shape (..., M, N).

It would be more consistent with the rest of the API to have axis1=-2, axis2=-1 to handle batches of matrices correctly by default.

@leofang
Copy link
Contributor

leofang commented Jul 26, 2021

Wouldn't it be better to just remove axis1 and axis2 entirely and always use the last two axes? This makes it more consistent with other linalg APIs, and is mathematically more intuitive. Users who need to compute trace/diagonal along arbitrary two axes can always swap the axes (related: #228) first before calling the API.

@lezcano
Copy link
Contributor Author

lezcano commented Jul 26, 2021

At least in PyTorch, it would be as efficient to transpose the diagonals to the back and then call diagonal than it would be calling it with this API. Now, this API may be convenient for some applications? That I do not know. I have never used diagonal with anything other than -2, -1, but my experience is very biased towards working with matrices.

@leofang
Copy link
Contributor

leofang commented Jul 26, 2021

In CuPy it's also as efficient, given that transpose/swapaxes only return views (with different strides). I guess this suggests for both PyTorch and CuPy (and likely NumPy) removing axis1/axis2 is safe. 🙂

@rgommers
Copy link
Member

trace is not heavily used (there's no data in https://github.com/data-apis/python-record-api, and a search in SciPy seems to confirm it's not used there at all). I suspect when it is used, it's indeed with -2, -1.

Wouldn't it be better to just remove axis1 and axis2 entirely and always use the last two axes? This makes it more consistent with other linalg APIs, and is mathematically more intuitive.

+1 makes sense to me

@rgommers
Copy link
Member

Fixed by gh-241, which removes the keywords. Thanks @lezcano and @leofang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: Linear Algebra Linear algebra.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants