-
-
Couldn't load subscription status.
- Fork 5.7k
Make pinv work for Adjoint #29837
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
Make pinv work for Adjoint #29837
Conversation
| Sinv = zeros(Stype, length(SVD.S)) | ||
| index = SVD.S .> rtol*maximum(SVD.S) | ||
| Sinv[index] = one(Stype) ./ SVD.S[index] | ||
| Sinv[findall(.!isfinite.(Sinv))] .= zero(Stype) |
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.
Why is this change needed?
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.
It's not needed for this change. However, zero(Stype) isn't needed since the array is already allocated with Stype elements and 0 is easier to read than zero(Stype).
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.
Aren't there places where zero(T) works but convert(T, 0) fails? I'd let sleeping dogs lie here, especially since we also use one and zeros immediately surrounding this line.
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.
There are but they don't apply here since Stype is the type the singular values. It's mainly for non-number types like arrays that you can't rely on converting from 0. This change is unrelated to the PR so I've removed it.
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.
I had also been thinking of unitful numbers but hadn't worked out if/how unitful SVDs work. In any case, thanks for indulging my conservativeness.
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.
That is true. The singular values have units so that would fail. Unfortunately, it would be a bit complicated to make the svd solver in GenericLinearAlgebra work for Unitful.Quantity because it's only a subtype of Number.
Fixes #29723
Fixes #29723 (cherry picked from commit a472bc7)
Fixes #29723 (cherry picked from commit a472bc7)
Fixes #29723 (cherry picked from commit a472bc7)
Fixes #29723 (cherry picked from commit a472bc7)
Fixes #29723 (cherry picked from commit a472bc7)
Fixes JuliaLang/LinearAlgebra.jl#578