Repository navigation
[Consistency] Default axes in linalg.diagonal and linalg.trace. #215
Description
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.
Activity
leofang commentedon Jul 26, 2021
Wouldn't it be better to just remove
axis1andaxis2entirely 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 commentedon 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 commentedon 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/axis2is safe. 🙂rgommers commentedon Jul 27, 2021
traceis 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.+1 makes sense to me
rgommers commentedon Aug 19, 2021
Fixed by gh-241, which removes the keywords. Thanks @lezcano and @leofang