-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Switch SVD on CPU from gesvd to gesdd #11194
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
Conversation
|
I have got some times with me (10 runs w/ 10 loops each)
|
|
The performance speedup is great, @vishwakftw! Could you benchmark some smaller matrices? In particular, I'm reading https://groups.google.com/forum/#!topic/julia-dev/mmgO65i6-fA and from their experience it sounds like there is some value in having a second svd call that uses gesvd, but I'm curious what your benchmarking shows about that. edit: Oh, I found the discussion in #11174 where @soumith linked to the link I sent. Please ignore my comment :) |
|
I believe this reiterates the importance of shifting to
|
ssnl
left a 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.
looking good except for 1 nit
torch/_torch_docs.py
Outdated
| .. note:: The implementation of SVD on CPU uses the LAPACK routine `?gesdd` (a divide-and-conquer | ||
| algorithm) instead of `?gesvd` for speed. However, the SVD on GPU uses the MAGMA routine | ||
| `gesvd` since it takes up lesser workspace area. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
I have got some more times with me (10 runs w/ 1000 loops each)
cc: @zou3519 From this, as the matrix size starts diminishing, the advantage fades away. |
|
@vishwakftw those times look great |
|
BTW, I would also check if the result has any NaN's. I found Intel's gesdd erroneously produced NaNs when gesvd didn't. I feel like some of the edge cases are likely to come up on more ill-conditioned matrices. For instance here's a particular (very ill-conditioned) matrix that breaks TensorFlow's SVD routine |
|
@ssnl is this good to go? |
|
@yaroslavvb The example you have given seems to not break the current implementation in this PR. There were no |
|
@pytorchbot retest this please |
facebook-github-bot
left a 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.
SsnL has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a 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.
soumith has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: - Added a note to the doc string for `svd`. Pull Request resolved: pytorch/pytorch#11194 Differential Revision: D9683250 Pulled By: soumith fbshipit-source-id: 2d2c120be346122afa333629c0516a5c9dbb406f
Summary: - Added a note to the doc string for `svd`. Pull Request resolved: pytorch#11194 Differential Revision: D9683250 Pulled By: soumith fbshipit-source-id: 2d2c120be346122afa333629c0516a5c9dbb406f
svd.