Backward operation of torch.eig for real eigenvalues#33090
Backward operation of torch.eig for real eigenvalues#33090mfkasim1 wants to merge 3 commits intopytorch:masterfrom mfkasim1:deriveig2
Conversation
💊 CircleCI build failures summary and remediationsAs of commit 5d676ee: Commit 5d676ee was recently pushed. Waiting for builds... 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. This comment has been revised 2 times. |
albanD
left a comment
There was a problem hiding this comment.
This looks quite good!
Small details about test and perf but this is almost ready to go.
Will this need an update when the batch eig lands? Or are these formulas going to work for both?
|
Thanks for the review, @albanD. The computation in this PR should not be changed when my another PR lands. The way I wrote it is by following closely |
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.
IFU 20200210 #2: Backward operation of torch.eig for real eigenvalues (pytorch#33090)
Summary: Another pull request to follow up issue pytorch#32531. Here I implemented the backward operation for `torch.eig` with a condition that all the eigenvalues are real. This pull request is independent of my another pull request pytorch#32932, which means that there is no dependency between this PR and my another PR. Pull Request resolved: pytorch#33090 Differential Revision: D19814347 Pulled By: albanD fbshipit-source-id: 2fae30964e97987abb690544df8240aedeae56e8
Summary: Related: #33090 I just realized that I haven't updated the docs of `torch.eig` when implementing the backward. Here's the PR updating the docs about the grad of `torch.eig`. cc albanD Pull Request resolved: #47598 Reviewed By: heitorschueroff Differential Revision: D24829373 Pulled By: albanD fbshipit-source-id: 89963ce66b2933e6c34e2efc93ad0f2c3dd28c68
|
|
||
| auto gvortho = gv - at::sum(gv * v, /*dim=*/-2, /*keepdim=*/true) * v; | ||
| auto B = hm * at::matmul(vt, gvortho); |
There was a problem hiding this comment.
@mfkasim1 , could you please explain this line where you subtract contributions of v from gv? It does not seem to agree with the reference you mentioned above.
There was a problem hiding this comment.
I actually derived the expression myself, but I didn't have the time to write it down, so I just put the closest available literature in the code.
There was a problem hiding this comment.
The derivations are different, so I do not know which method to trust, but I do not immediately see any issues in the reference. So, then, how did you get that gvortho?
There was a problem hiding this comment.
I'm not sure if they are actually different or actually the same with just different expressions.
I have put my expression into gradcheck and gradgradcheck and it passes.
I will write the derivation down if I got time.
There was a problem hiding this comment.
They seem to different, in your derivation you subtract from the gv the projection of gv into v, and they do not do it in the reference. The parts with the eigenvalue contribution seem to match.
There was a problem hiding this comment.
Ah, if I remember it correctly, I subtract it because of the constraint of the eigenvectors to have norm = 1.
I probably did the similar trick from this paper (eq. 3.23-3.25)
There was a problem hiding this comment.
All right, now I see. I did check the output of torch.eig it indeed produces normalized eigenvectors. Awesome, thank you!
|
Is this feature updated to pytorch 1.7.1? |
|
@kimjw0623 yes, it should be. You can check. |
Another pull request to follow up issue #32531.
Here I implemented the backward operation for
torch.eigwith a condition that all the eigenvalues are real.This pull request is independent of my another pull request #32932, which means that there is no dependency between this PR and my another PR.