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.cholesky #46083
Added linalg.cholesky #46083
Changes from 14 commits
6b7f14a
f7a08f4
32e10f8
3c4d5a4
307020e
9e9e0c0
ae4f3ee
1798832
e74de2c
9343678
604f0a8
5f80042
da4e88b
efb725c
6297b6b
709273b
21cfca0
31cbe75
df0172e
b54c7f8
7878183
0d4a8c7
e4832d3
e800b97
8b58586
7aabce3
12d11ce
97501c6
29f94c9
00c41ed
34cef7d
5d2230d
63e922d
6d64067
40935f1
aaad340
1d67cd0
22be1a5
d547b63
fe89410
9e2cfb4
30239f2
f723421
1b46c19
10d276a
e224c97
048316d
0a13e1d
173fea1
b26f52c
ba3708e
5f0abff
b670f26
f2ee1f6
344053f
869b40a
d52b83b
c98076b
287c878
03b29a3
88e23c3
eb507e5
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.
The absolute tolerance for float32 is surprisingly large. What's going on there?
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 assume that NumPy could silently convert the input to fp64 and then back to fp32.
CPU tests pass for
1e-3
tolerance. GPU passes only with1e-2
. It doesn't mean that the decomposition is incorrect, it's all valid, but the individual entries can differ a bit across different libraries/environments.A similar thing was observed for
inverse
#45034, where the results of cuSOLVER and MAGMA differ a lot for fp32.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 your analysis is probably correct. Would you verify what dtype NumPy computes in and a comment here explaining the absolute tolerance value?
How significant a loss of precision is 1e-2 here? In terms of relative tolerance, I mean?
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.
Does this produce the aforementioned RuntimeError? If so, then I think it's probably better to use a
with self.assertRaisesRegex(...):
, rather thanexpectedFailure
.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 idea to use an xfail test (
unittest.expectedFailure
) because that is a bug we can't fix right now.I would expect to use
self.assertRaisesRegex(...)
(and consequently mark the test as 'pass') for the intended errors that are not bugs.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.
Maybe would also be good to mention that once jit and CUDA complex support are added, this test should be moved to
test_autograd_and_jit
. Or if the tests intest_autograd_and_jit
have been moved intocommon_methods_invocations.py
, it should be moved there.Jit and CUDA complex support can probably be done in future PRs.
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 test for
torch.cholesky
fromtest/test_autograd.py
is not part ofcommon_methods_invocations.py:method_tests
. Maybe the reason is that it requires making the input hermitian for gradcheck to work correctlyfunc(x)
is being tested nottorch.linalg.cholesky(x)
directly. So I don't know whether it can be moved later totest_autograd_and_jit
/common_methods_invocations.py
.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.
As far as I understand, I don't think that prevents us from putting this in
common_methods_invocations.py:method_tests
. The second element of an entry inmethod_tests
can be a contructing function, for instance: https://github.com/pytorch/pytorch/blob/master/torch/testing/_internal/common_methods_invocations.py#L521So I think it would be possible to make a constructing function for
cholesky
too.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.
@anjali411 has been working on better gradcheck support for complex, I'm not sure about the gradgradcheck status. HOWEVER, I think @peterbell10 found a nice workaround for this issue. @peterbell10, can you comment?
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.
gradgradcheck
works with #45737. Once it's is merged, I'll update the code here.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.
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.
Would you move this test to test_linalg.py? It will need a new name, like "test_torch_cholesky" or "test_old_cholesky."
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.
Also, is there a change to this test that would demonstrate your lda fix?
Also also, if you spot other Cholesky tests let's move them, too.
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 combining the first few paragraphs would be helpful. What about something like:
"Computes the Cholesky decomposition of a Hermitian positive-definite matrix or the Cholesky decompositions of a batch of such matrices. If the matrices are real-valued then each of their Cholesky decompositions can be written as A = LL^t, for a lower triangular matrix L, and this function returns the matrix L for each input matrix. If the matrices are complex-valued then their Cholesky decompositions are A = L @ L.H for a lower triangular matrix L, where L.H is the conjugate transpose of L."
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 note also needs to account for the batching behavior. Maybe something like:
"If :attr:`input` is not a Hermitian positive-definite matrix, or if it's a batch of matrices and one of them is not a Hermitian positive-definite matrix, then a RuntimeError will be thrown."
As for the error behavior: does it specify which matrix in the batch was discovered to not be a Hermitian positive-definite matrix, too? Another option here would be to not elaborate about the content of the error message.
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.
Batch is also specified in the error message.
https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/LinearAlgebraUtils.h#L104
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.
Great; let's be clear about that (and maybe even add a test for it erroring on the correct batch element).
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 warning that this causes cross-device synchronization when called on CUDA inputs
cc @heitorschueroff who's looking into cross-device data movement in operations that use MAGMA
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.
Calling the tensor "A" here isn't helpful and possibly misleading if input is a batch of matrices. Maybe you can say, "a Hermitian positive-definite matrix or batch of such matrices."?
If multiple matrices are passed as input, does each of them have to have the same size? That requirement (or lack thereof) should be mentioned somewhere in the documents.
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'll remove the "A".
The input tensor must have the shape of the form
(*, n, n)
, the last two dimensions are equal, i.e. batch of square matrices. There is a check for that and tests. I could add that matrices should be square, but that's already implicit by calling a matrix positive-definite.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.
And it's not technically possible to pass multiple matrices as the input of different size, as we don't support list of tensors and consider the last two dimensions of a tensor as a matrix we process.
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.
Right. Good point. I've been in too many discussions about nested tensors, which can be ragged, recently. My mistake.
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 what this function does and how to call it.
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 the signature of this function should be:
(matrix_size, *, batch_dims, dtype, device)
.Since it's an internal testing function it probably shouldn't have default values. Each caller should be responsible for correctly setting dtype and device.
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.
Okay, I can change that. I was just following the other existing functions.
In fact, all calls to
random_symmetric_pd_matrix
can be safely replaced byrandom_hermitian_pd_matrix
, probably in a follow-up PR.