Make torch.svd return V, not V.conj() for complex inputs#51012
Make torch.svd return V, not V.conj() for complex inputs#51012IvanYashchuk wants to merge 10 commits intopytorch:masterfrom
Conversation
|
This link points to the current documentation. |
💊 CI failures summary and remediationsAs of commit 7fc28c5 (more details on the Dr. CI page):
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
XLA's |
Continuing to use at::svd to implement pinverse (for now) sounds good. |
Just so I'm confident I understand what's going on here:
Is that correct? Follow-up question: how long has torch.svd been returning V and not V^H for complex inputs? |
|
LAPACK and MAGMA return V.transpose() for real inputs and V.conj().transpose() for complex inputs.
torch.svd had been returning V.conj(), not V since the 3rd of October. Introduced in this PR #45795 |
Got it, thank you for clarifying. This all makes sense now. Looks like this support is part of PyTorch 1.7.1, so this will be a BC-breaking change. Worse, it's a silent correctness issue. I expanded the PR summary with a BC-breaking note reflecting the current state of this PR (please correct me if I'm still missing something). It would be safer if we could disallow complex inputs to torch.svd, even if internally we rely on at::svd with complex inputs. I can't think of any reasonable way to do that, however. Luckily torch.svd is being deprecated and this functionality was never documented, so it may be OK to change this. |
There was a problem hiding this comment.
Thanks for the cleanup @IvanYashchuk !
The PR looks good to me.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Codecov Report
@@ Coverage Diff @@
## master #51012 +/- ##
==========================================
+ Coverage 80.70% 80.91% +0.21%
==========================================
Files 1926 1926
Lines 210012 210020 +8
==========================================
+ Hits 169485 169933 +448
+ Misses 40527 40087 -440 |
Summary: **BC-breaking note:** torch.svd() added support for complex inputs in PyTorch 1.7, but was not documented as doing so. The complex "V" tensor returned was actually the complex conjugate of what's expected. This PR fixes the discrepancy. This will silently break all users of torch.svd() with complex inputs. **Original PR Summary:** This PR resolves pytorch#45821. The problem was that when introducing the support of complex inputs for `torch.svd` it was overlooked that LAPACK/MAGMA returns the conjugate transpose of V matrix, not just the transpose of V. So `torch.svd` was silently returning U, S, V.conj() instead of U, S, V. Behavior of `torch.linalg.pinv`, `torch.pinverse` and `torch.linalg.svd` (they depend on `torch.svd`) is not changed in this PR. Pull Request resolved: pytorch#51012 Reviewed By: bdhirsh Differential Revision: D26047593 Pulled By: albanD fbshipit-source-id: d1e08dbc3aab9ce1150a95806ef3b5da98b5d3ca



BC-breaking note:
torch.svd() added support for complex inputs in PyTorch 1.7, but was not documented as doing so. The complex "V" tensor returned was actually the complex conjugate of what's expected. This PR fixes the discrepancy.
Note that this means this PR silently breaks all current users of torch.svd() with complex inputs.
Original PR Summary:
This PR resolves #45821.
The problem was that when introducing the support of complex inputs for
torch.svdit was overlooked that LAPACK/MAGMA returns the conjugate transpose of V matrix, not just the transpose of V. Sotorch.svdwas silently returning U, S, V.conj() instead of U, S, V.Behavior of
torch.linalg.pinv,torch.pinverseandtorch.linalg.svd(they depend ontorch.svd) is not changed in this PR.