Skip to content
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

Implement gradient for SVD #614

Merged
merged 1 commit into from
Apr 28, 2024
Merged

Conversation

jessegrabowski
Copy link
Member

@jessegrabowski jessegrabowski commented Jan 23, 2024

Implement gradients for SVD

Description

I just copied the implementation from autograd here. The base case compute_uv=False, full_matrices=False works and test passes.

Marked as a draft because I'm not sure how to handle multiple output gradients in a test. I'm not sure my copy/paste job is correct until I verify (there are some slight differences between our SVD and the reference I was following -- I think we return V, but they returned V.conj().T. This is a numpy vs matlab thing I think.

Even if we compute all the matrices, we only get a gradient for the singular values. Is there a way to return [NoGrad, GradientGraph, NoGrad]?

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@jessegrabowski jessegrabowski added enhancement New feature or request gradients linalg Linear algebra labels Jan 23, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.81%. Comparing base (eb18f0e) to head (196b5e4).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
+ Coverage   80.78%   80.81%   +0.02%     
==========================================
  Files         162      162              
  Lines       46757    46800      +43     
  Branches    11440    11449       +9     
==========================================
+ Hits        37773    37820      +47     
+ Misses       6735     6733       -2     
+ Partials     2249     2247       -2     
Files Coverage Δ
pytensor/tensor/nlinalg.py 95.22% <100.00%> (+0.46%) ⬆️

... and 2 files with indirect coverage changes

pytensor/tensor/nlinalg.py Outdated Show resolved Hide resolved
pytensor/tensor/nlinalg.py Outdated Show resolved Hide resolved
@ricardoV94
Copy link
Member

Is there a way to return [NoGrad, GradientGraph, NoGrad]?

Yes, but what's the reason for NoGrad? See different flags in section: https://pytensor.readthedocs.io/en/latest/extending/op.html#gradient

@ricardoV94
Copy link
Member

@jessegrabowski
Copy link
Member Author

jessegrabowski commented Feb 13, 2024

I'm very confident the gradients are implemented correctly, but I'm getting shape errors in the gradient check for the non-square case. As usual, there's also no complex support -- I didn't even bother to add the xfail tests this time, but I could.

I am hoping to get a 2nd pair of eyes on it to see if I'm missing something obvious. I wonder if it's related to the use of shared variables in verify_grad? There's no need for it -- we should just pass the random project matrices as arguments to the compiled function.

@ricardoV94
Copy link
Member

I wonder if it's related to the use of shared variables in verify_grad? There's no need for it -- we should just pass the random project matrices as arguments to the compiled function.

That shouldn't be a problem. I'll try to have a look, but that utility take a while to debug.

@jessegrabowski
Copy link
Member Author

jessegrabowski commented Feb 13, 2024

False alarm, I had a dumb mistake in the gradients. This should be good to go now.

I still hate shared variables.

pytensor/gradient.py Outdated Show resolved Hide resolved
@ricardoV94 ricardoV94 merged commit b7c952b into pymc-devs:main Apr 28, 2024
55 checks passed
@jessegrabowski jessegrabowski deleted the svd-grad branch October 8, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request gradients linalg Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement gradient of SVD
3 participants