-
Notifications
You must be signed in to change notification settings - Fork 24.4k
pinv: forward/backward AD which is Frechet-defined in a rank-preserving neighborhood. #66092
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
rank-preserving neighborhood.
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slow For more information, please take a look at the CI Flow Wiki. |
# Only large tensors show issues with implicit backward used prior to | ||
# explicit backward implementation. |
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 note: large tensors of low rank. In my environment I had to create a 1-rank 30x30 matrix to see issues with repeated "zeros" in the backward of SVD.
not sure appropriate FB reviewer has been tagged yet |
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 2b5431e (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
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.
Nice! Thanks both for finding the slick formula for this backward and the rather compact implementation!
# Note that by making the columns of `a` and `b` orthonormal we make sure | ||
# that the product matrix `a @ b.t()` has condition number 1. |
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.
Nice! This saves us a lot of pain in future debugging.
Now, this note is slightly incorrect. The resulting matrix will have singular values 0 and 1, so the condition number will be infinite! Perhaps you mean that it has condition number 1 when restricted to its image?
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.
Yes, exactly in the image, correct, so that pinv is stable.
sample_inputs_func=sample_inputs_linalg_pinv_singular, | ||
# Only large tensors show issues with implicit backward used prior to | ||
# explicit backward implementation. | ||
decorators=[slowTest, skipCUDAIfNoMagmaAndNoCusolver, skipCUDAIfRocm, skipCPUIfNoLapack], |
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.
Is the slowTest decorator working as expected here?
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.
Yes!
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.
@albanD It will apply the slowTest decorator to EVERY test generated by this OpInfo
Cool! Do you have before/after perf numbers for the autograd, @nikitaved? |
…taved/pinv_backward
…taved/pinv_backward
@mruberry, I did run some benchmarks and surprisingly this PR also improves performance. This PR, cpu float32:
Master, cpu float32:
This PR, cuda float32:
Master, cuda float32:
|
…taved/pinv_backward
Faster and correct! There's no better combination than that :) |
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.
Looks good. Can you fix the last lint (EDIT: Ho it looks like the job itself failed...)and I'll merge this.
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Fixes #65911. Also enables complex support/tests for
linalg_pinv
in OpInfo.cc @ezyang @albanD @zou3519 @gqchen @pearu @nikitaved @soulitzer @lezcano @Varal7 @jianyuh @mruberry @walterddr @IvanYashchuk @xwang233