Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added linalg.eigh, linalg.eigvalsh #45526
Added linalg.eigh, linalg.eigvalsh #45526
Changes from 13 commits
018604f
4662eb7
bdcda38
b6f814c
781fb2c
8f6325b
6654f4c
ecd0c80
c871eb6
7a15c47
432743f
5a94008
e8180d8
4f8542c
629d934
e4d5a23
62f3e29
f151bfa
9e94c8b
3ce51ca
ec8f824
0c0dbac
b55f824
3c08f0f
8fdf8b2
4477796
87b7c22
794d7cd
4e405bb
a3adeb4
93b68c4
685f383
f880538
0be9cac
efdbb61
0ce7224
a8f8366
434aa9a
9ca046a
d9df1b5
890478d
9abfb24
8a775fd
0a8669d
2603839
c65451b
78f6b4e
26971ec
5ed0159
dbc7a72
2911420
85c6c80
bd41343
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment describing the function and how it's intended to be called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here explaining that the use of imprecise types, like int, is consistent with the LAPACK interface (otherwise we'd use a precise type, like int32_t).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: if we were being extremely correct we would add a using declaration to make it easy to change this int type later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this kind of type change should happen at once for all functions that call to LAPACK. There should be
lapack_int
that depends on the type of the linked LAPACK or only 64-bit indexing should be used by default and only 64-bit version of LAPACK allowed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you and the comment looks great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here explaining what triggers this return, and what callers should do if this is triggered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up question:
In the future we expect to add
_ex
variants of functions in linear algebra that have additional error information. For example, let's say we havetorch.linalg.foo
that computes thefoo
operation on a non-singular matrix. If the matrix is singular, however, then the math library we're using returns error information on a device tensor explaining that the matrix is singular.On the CPU, torch.linalg.foo(cpu_matrix) can throw an error when a singular tensor is encountered. On CUDA, however, translating the device tensor containing the error information requires a cross-device sync, which we try to avoid. torch.linalg.foo(cuda_matrix) should perform and document this sync, however.
torch.linalg.foo_ex, however, will return both device tensors and not perform the error check. This allows the user to check the error themselves, and avoid an immediate cross-device sync on CUDA if they want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current behavior for linalg (LAPACK/MAGMA based) functions to raise an error if something goes wrong or input doesn't satisfy some requirement therefore all batched CPU functions return early since further computations will be wasted anyway. In the future, this early return should be removed.
What does
_ex
suffix mean?I agree that this kind of change will improve performance. I think the default behavior should be the same on CPU and CUDA, so CPU shouldn't raise an error, even though it's cheap to check for it, if CUDA doesn't.
What do you think about instead of introducing new functions with
_ex
suffix in Python interface modify existing ones to accept optionalinfo=
argument, similar toout=
, that will be filled with the returned LAPACK/MAGMA/cuSOLVER error codes for the user to check if he wants to.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good for now.
"Extra."
Agreed. Both the CPU and CUDA ex variants will not perform their own error checking and return the info tensor for callers to evaluate.
This is a really good idea and we did consider an approach like this, but there are technical issues with this approach that make it prohibitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function causes a cross-device sync on CUDA, right? If so, this PR should add a warning here explaining that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That warning can be combined with a warning that this function will throw an error if a tensor does not meet the requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a note on cross-device synchronization for CUDA.
This function doesn't throw errors if the input is not Hermitian or symmetric because of
The documentation says the error code can be >0 indicating that the algorithm didn't converge. But I don't know what kind of input is needed to make it fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... is our failure behavior consistent with NumPy's, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. NumPy raises
LinAlgError("Eigenvalues did not converge")
forinfo > 0
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UPLO argument needs to be mentioned in the description of the function, too. This should be more specific, too. "considers" is too vague a term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the main text, it says now "only the lower triangular portion of these matrices is used by default (
UPLO = 'L'
) in computations".I changed "considers" -> "controls whether to use upper-triangular or lower-triangular part of :attr:
input
in the computations."