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
Implement torch.linalg.svd #45562
Implement torch.linalg.svd #45562
Changes from 32 commits
3923d4e
130cd04
f39dc15
e16fde1
a14c2fb
675e122
91ec980
5886d58
f763c8d
b319768
1817211
97662da
25caa20
f056875
edca9df
0462f79
e900ad2
e8ce282
25b6fef
c7bfc07
9bcf00b
7342ece
25752e6
10a66d7
b1c3cfb
6f480c2
4038ace
a224dbd
abf9baf
0c0e8c6
37ec294
60e463f
2433399
4b4076d
5fa4efe
f16f6e7
a008c53
989505f
80e18d2
71d4db1
c9c60c2
7b70521
e25de98
5dc359d
50a28ae
8b30bcb
b87b765
22b17c0
cb9de75
a349569
e30248e
3ddb6ac
823e6a8
52aadbe
ea0aca4
9bc46f6
da1f2ae
5445d3f
6d04a6e
a2e2781
bcf7461
ba92c1a
fa927d0
a866c67
9d9cd03
c3e4de6
ec87163
958321e
944fa6a
8dbbfe5
be456ab
e483647
24c6506
c39f2ef
81510d5
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.
See note on error-checking out above. Here we should require same dtype and same device for now, and use resize_output instead of resize_as_.
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.
done in commit e483647.
I noticed that
resize_output
doesn't check the device, so I wrote the check manually: so, what is the convention for output tensors? Is it generally allowed to pass output tensors on different devices? If not, why the check is not done byresize_output
itself?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 is an excellent question. I actually wrote a brief description of out= handling in the Developer FAQ: https://github.com/pytorch/pytorch/wiki/Developer-FAQ#how-does-out-work-in-pytorch. I think it's currently a gap in the tools PyTorch provides that we require some operations to implement this check manually.
@ezyang is actually developing a new architecture that I think solves this issue. Maybe we should extend
resize_output
, too. Currently it doesn't have the device information necessary to implement this check. @heitorschueroff actually had that same idea recently.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.
thank you for the answer, I have a better picture now
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 test and test_svd_no_compute_uv need to test more sizes, include a batched input.
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 it? The actual logic of svd is inside
_svd_helper
, which is already tested explicitly by_test_svd_helper
and all the tests which calls it. In my idea, thetest_linalg_svd_*
are meant to test the only actual differences, i.e. the signature and return types.But I am fine to duplicate all the tests if you think it's a good idea.
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.
Don't use torch.Tensor(), which is deprecated. Instead use torch.tensor/torch.empty/torch.empty_like.
Does this mean that an empty CPU tensor is an acceptable out= value when running on CUDA? torch.linalg.svd should verify that the tensors passed to out= are valid whether compute_uv is True or False.
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.
currently yes, but that's true for most (if not all) operators defined
BatchLinearAlgebra.cpp
. E.g.torch.eig
,torch.svd
,torch.solve
, and in general all operators which uses the patternout.resize(X).copy_(...)
.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.
Yikes. I think you're right and that's a bug. Let's get this case correct and I've updated #49468 to include a bullet point for testing this behavior.
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.
It looks to me that the best way to implement this behavior would be to add this extra check to
resize_output
, and make sure to use it everywhere.I'm happy to work on this but I would prefer to do this in a separate PR, since it probably involves touching a lot of different code, introduce new tests, etc.
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.
All the svd tests should go in test_linalg.py, even the ones for torch.svd
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.
done in 5fa4efe.
However it is a bit weird now: there is
test_linalg.py:test_svd
which does NOT testtorch.linalg.svd
but it teststorch.svd
.I tried to improve the situation by renaming the new tests into
test_linalg_svd_*
and to put comments to divide the two sections.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 could be:
"PyTorch's implementation of SVD uses LAPACK's (on CPU) or MAGMA's (on CUDA) `?gesdd` (a divide-and-conquer algorithm) instead of `?gesvd` for speed."
What's up with the question marks before gesdd and gesvd?
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.
Just for the information.
MKL documentation for LAPACK uses the question marks in place of a letter indicating what datatype the function operates on (
{'s','d','c','z'} -> '?'
).https://software.intel.com/content/www/us/en/develop/documentation/mkl-developer-reference-fortran/top/lapack-routines/naming-conventions-for-lapack-routines.html
Netlib documentation for LAPACK the first datatype letter is replaced with 'x'
https://www.netlib.org/lapack/lug/node24.html
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.
Oh I see. I suppose we could remove the question mark or replace it with the types we support in braces, like: {a, b}gesdd, but the current presentation also seems fine.
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.
A quick search in the docs seems to suggest that we are not using a consistent naming scheme. E.g.:
cholesky_inverse
explicitly lists all variations by sayingdpotri and spotri
toch.geqrf
mentions bothgeqrf
and?geqrf
in the same docstringorgqr
the sameThere 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.
It's not surprising we're inconsistent, and since we're inconsistent this PR can do whatever you like.
cc @heitorschueroff, too. We should review the linear algebra documentation for consistency ahead of the 1.8 release.
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.
It is technically true that the "full_matrices" argument affects the shape of `U` and `V`, but there's probably a better way to describe its effect. This also needs to include the default value (see below).
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 tried to improve but I'm not sure I like the result:
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.
"argument" -> "arguments"
This is interesting. So this function will not set them to be empty tensors in this case?
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 of the current code, yes. It seems weird to check and resize the shape of them if they are ultimately ignored.