JuliaStats / PDMats.jl

Uniform Interface for positive definite matrices of various structures
Other
104 stars 43 forks source link

Improve constructor `PDMat(::Cholesky)` #165

Closed devmotion closed 2 years ago

devmotion commented 2 years ago

This PR fixes #155. This PR fixes #137.

On the master branch

julia> using PDMats, StaticArrays, BenchmarkTools, LinearAlgebra

julia> A = SMatrix{3,3}(rand(3, 3));

julia> B = A * A' +  I;

julia> @btime PDMat($(cholesky(B)));
  45.653 ns (1 allocation: 128 bytes)

With this PR

julia> using PDMats, StaticArrays, BenchmarkTools, LinearAlgebra

julia> A = SMatrix{3,3}(rand(3, 3));

julia> B = A * A' +  I;

julia> @btime PDMat($(cholesky(B)));
  9.706 ns (0 allocations: 0 bytes)
codecov-commenter commented 2 years ago

Codecov Report

Merging #165 (feaafb2) into master (ec732d6) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #165   +/-   ##
=======================================
  Coverage   90.19%   90.19%           
=======================================
  Files           8        8           
  Lines         418      418           
=======================================
  Hits          377      377           
  Misses         41       41           
Impacted Files Coverage Δ
src/pdmat.jl 97.87% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ec732d6...feaafb2. Read the comment docs.

matbesancon commented 2 years ago

a method was missing for lmul! in Julia 1.0, I'd suggest requiring the LTS, it was bound to happen at some point

  MethodError: no method matching lmul!(::Adjoint{Float64,UpperTriangular{Float64,Diagonal{Float64,Array{Float64,1}}}}, ::SparseMatrixCSC{Float64,Int64})

the LTS fail is unrelated again

devmotion commented 2 years ago

a method was missing for lmul! in Julia 1.0, I'd suggest requiring the LTS, it was bound to happen at some point

  MethodError: no method matching lmul!(::Adjoint{Float64,UpperTriangular{Float64,Diagonal{Float64,Array{Float64,1}}}}, ::SparseMatrixCSC{Float64,Int64})

I don't think this warrants an update of the Julia compat entry. This PR is an improvement even for Julia 1.0 (fixes #155 - can be seen e.g. since CI passes for the first commit), just one specific example (namely the one in #137 - test for it added in the second commit) is still broken. However, on Julia <= 1.3 that example fails with the master branch as well due to the same problem:

julia> using LinearAlgebra

julia> x = rand(10, 10);

julia> A = Diagonal(x' * x);

julia> Matrix(cholesky(A)) # used on the master branch
ERROR: MethodError: no method matching lmul!(::Adjoint{Float64,UpperTriangular{Float64,Diagonal{Float64,Array{Float64,1}}}}, ::SparseArrays.SparseMatrixCSC{Float64,Int64})
...

julia> AbstractMatrix(cholesky(A)) # used in this PR
ERROR: MethodError: no method matching lmul!(::Adjoint{Float64,UpperTriangular{Float64,Diagonal{Float64,Array{Float64,1}}}}, ::SparseArrays.SparseMatrixCSC{Float64,Int64})
...

So in my opinion, since already on the master branch that specific example fails due to this bug in Julia itself and hence the underlying issue in PDMats (and its fix here) are not even relevant, it is fine to just not fix that example on Julia <= 1.3 and instead tell users that want to use cholesky(Diagonal(...)) inputs to update their Julia version.

the LTS fail is unrelated again

The unrelated test failures occur with Julia nightly (caused by BandedMatrices, will be fixed by https://github.com/JuliaMatrices/BandedMatrices.jl/pull/258), LTS should be fine (but is not part of the CI tests, I opened #166).