Updated derivative rules for complex QR decomposition#48489
Updated derivative rules for complex QR decomposition#48489IvanYashchuk wants to merge 8 commits intopytorch:masterfrom
Conversation
💊 CI failures summary and remediationsAs of commit 630991a (more details on the Dr. CI page):
1 failure not recognized by patterns:
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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 17 times. |
Yes, I think it carries the same set of limitations as the previous implementation because the new one still doesn't use pivoting or any other techniques for improving numerical stability. |
Codecov Report
@@ Coverage Diff @@
## master #48489 +/- ##
===========================================
+ Coverage 24.94% 80.76% +55.81%
===========================================
Files 467 1867 +1400
Lines 58746 201579 +142833
===========================================
+ Hits 14656 162809 +148153
+ Misses 44090 38770 -5320 |
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.
|
@IvanYashchuk this has merge conflict with master because the other PR landed first and modified the exact same lines. |
|
@albanD, done. |
|
Thanks for the quick rebase! The lint is not happy with the extra space in the test file though |
|
Oh, should be fixed now. |
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.
|
Thanks! |
|
Resolved another merge conflict. |
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.
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.
|
@IvanYashchuk sorry about this one, it keeps failing to land and then have merge conflicts... |
Summary: Updated `qr_backward` to work correctly for complex-valued inputs. Added `torch.qr` to list of complex tests. The previous implementation for real-valued differentiation used equation 42 from https://arxiv.org/abs/1001.1654 The current implementation is a bit simpler but the result for the real-valued input case is the same and all tests still pass. Derivation of complex-valued QR differentiation https://giggleliu.github.io/2019/04/02/einsumbp.html Ref. pytorch#33152 Pull Request resolved: pytorch#48489 Reviewed By: bdhirsh Differential Revision: D25272344 Pulled By: albanD fbshipit-source-id: b53c1fca1683f4aee5f4d5ce3cab9e559170e7cf
Updated
qr_backwardto work correctly for complex-valued inputs.Added
torch.qrto list of complex tests.The previous implementation for real-valued differentiation used equation 42 from https://arxiv.org/abs/1001.1654
The current implementation is a bit simpler but the result for the real-valued input case is the same and all tests still pass.
Derivation of complex-valued QR differentiation https://giggleliu.github.io/2019/04/02/einsumbp.html
Ref. #33152