GenericSVD.jl icon indicating copy to clipboard operation
GenericSVD.jl copied to clipboard

Unnecessarily specific argument types in bidiagonalize.jl

Open kagalenko-m-b opened this issue 5 years ago • 5 comments

While svd() routine accepts AbstractMatrix, it passes that argument to bidiagonalize_tall!(), which accepts Matrix. It seems to me that bidiagonalization routine can take the abstract type as well. That would allow using GenericSVD on wider range of values.

kagalenko-m-b avatar Jul 02 '20 14:07 kagalenko-m-b

I just hit a similar issue a computing partial SVD from IterativeSolvers:

julia> svdl(direction, nsv=1, vecs=:both)
ERROR: MethodError: no method matching bidiagonalize_tall!(::LinearAlgebra.Bidiagonal{BigFloat, Vector{BigFloat}})
Closest candidates are:
  bidiagonalize_tall!(::LinearAlgebra.Adjoint{T2, Matrix{T}}) where {T, T2} at /home/mbesancon/.julia/packages/GenericSVD/cT5Cu/src/bidiagonalize.jl:46
  bidiagonalize_tall!(::Matrix{T}) where T at /home/mbesancon/.julia/packages/GenericSVD/cT5Cu/src/bidiagonalize.jl:50
  bidiagonalize_tall!(::Matrix{T}, ::LinearAlgebra.Bidiagonal) where T at /home/mbesancon/.julia/packages/GenericSVD/cT5Cu/src/bidiagonalize.jl:18
Stacktrace:
 [1] generic_svdfact!(X::LinearAlgebra.Bidiagonal{BigFloat, Vector{BigFloat}}; sorted::Bool, full::Bool)
   @ GenericSVD ~/.julia/packages/GenericSVD/cT5Cu/src/GenericSVD.jl:30
 [2] svd!(X::LinearAlgebra.Bidiagonal{BigFloat, Vector{BigFloat}}; full::Bool, thin::Nothing, alg::Nothing)
   @ GenericSVD ~/.julia/packages/GenericSVD/cT5Cu/src/GenericSVD.jl:18
 [3] svd!(X::LinearAlgebra.Bidiagonal{BigFloat, Vector{BigFloat}})
   @ GenericSVD ~/.julia/packages/GenericSVD/cT5Cu/src/GenericSVD.jl:11
 [4] svd(M::LinearAlgebra.Bidiagonal{BigFloat, Vector{BigFloat}}; kw::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ LinearAlgebra /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/bidiag.jl:214
 [5] svd(M::LinearAlgebra.Bidiagonal{BigFloat, Vector{BigFloat}})
   @ LinearAlgebra /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/bidiag.jl:214
 [6] svdl_method!(log::ConvergenceHistory{false, Nothing}, A::Matrix{BigFloat}, l::Int64; k::Int64, j::Int64, v0::Vector{BigFloat}, maxiter::Int64, tol::Float64, reltol::Float64, verbose::Bool, method::Symbol, vecs::Symbol, dolock::Bool)
   @ IterativeSolvers ~/.julia/packages/IterativeSolvers/TpeDx/src/svdl.jl:192
 [7] svdl(A::Matrix{BigFloat}; nsv::Int64, k::Int64, tol::Float64, maxiter::Int64, method::Symbol, log::Bool, kwargs::Base.Iterators.Pairs{Symbol, Symbol, Tuple{Symbol}, NamedTuple{(:vecs,), Tuple{Symbol}}})
   @ IterativeSolvers ~/.julia/packages/IterativeSolvers/TpeDx/src/svdl.jl:168

matbesancon avatar Feb 18 '21 18:02 matbesancon

Can every subtype of AbstractMatrix be updated in place?

kagalenko-m-b avatar Feb 22 '21 08:02 kagalenko-m-b

No. There are many that cannot. E.g. StaticArrays.SMatrix

andreasnoack avatar Feb 22 '21 08:02 andreasnoack

So GenericSVD does not implement not in-place svd(). To enable it, probably it would be necessary to rewrite it into something like

function bidiagonalize_tall(A_in::Matrix{T}, A_out::Matrix{T}, B::Bidiagonal)
...
end
bidiagonalize_tall!(A::Matrix{T},B::Bidiagonal) = bidiagonalize_tall(A, A, B)

kagalenko-m-b avatar Feb 22 '21 09:02 kagalenko-m-b

Possibly but generally numerical linear algebra is super inconvenient for immutable arrays.

andreasnoack avatar Feb 22 '21 09:02 andreasnoack