-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
inv(F::SVD) specialized method #633
Comments
Shouldn't |
I guess that's a matter of definition. It does currently give you the inverse matrix as for all the other factorizations. In my own packages I define |
Generally, having the SVD of a matrix is much more powerful than having the matrix itself; I would much prefer to keep the inverse factorized as long as it generally works in places where multiplied out inverse matrix would work, which an SVD object generally should. |
It currently doesn't. For example, neither can I multiply a SVD with something nor add something to it. Also, getting the factorization of the inverse of a factorization can sometimes be harder (slower) than getting the inverse matrix. This isn't true for an SVD but should be for a QR if I'm not mistaken (please correct me if I am). Nonetheless, it makes sense to me to also have a function that calculates the inverse matrix of a factorization efficiently. |
In the case of easily invertible factorizations, I agree with @StefanKarpinski that we should keep factorization structure as long as possible. If the user wants a julia> @btime inv($F);
86.660 μs (13 allocations: 313.02 KiB) Now consider this: myinv2(F::SVD) = SVD(F.V, inv.(F.S), F.U') # perhaps we should check here if !iszero(F.S[end]) or so
julia> @btime myinv2($F);
98.675 ns (4 allocations: 960 bytes) Now, constructing the matrix: myinv3(F::SVD) = Matrix(myinv2(F))
@btime myinv3($F);
56.079 μs (13 allocations: 235.66 KiB) And for the original @btime myinv($F);
47.879 μs (6 allocations: 157.30 KiB) If |
Just to be clear, I do agree that keeping the factorized structure is generally superior.
It strikes me as odd to switch between returning a
As you showed, there is a performance difference, about double the number of allocations and about 17 % slowdown for a SVD (and it would be worse for other factorizations). We could try to argue that, in most performance critical cases, one shouldn't construct the full inverse matrix anyways. But I'm not sure whether this is enough to make the case. What about being explicit about it and having two distinct functions, maybe |
Hm, I think I agree, especially since the current behavior of julia> myinv(F::SVD) = (F.S .\ F.Vt)' * F.U'
myinv (generic function with 1 method)
julia> @btime myinv($F);
43.203 μs (4 allocations: 156.41 KiB) |
In any case, I think we should open a separate issue for the question of whether we should return factorization object in a future Julia version and just make this change as a straightforward performance improvement. |
Alright, I'll make a PR. (#32126) |
I realized that
inv
for aSVD
currently uses the genericinv(F::Factorization)
: https://github.com/JuliaLang/julia/blob/25c33e40c5abf5a43d8e45832a889755d6423f38/stdlib/LinearAlgebra/src/factorization.jl#L50With a simple specialized implementation like
I get the following speed up
Am I missing something or should this be added to
svd.jl
?The text was updated successfully, but these errors were encountered: