JuliaStats / PDMats.jl

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

Simplify `kron(::PDiagMat, ::PDiagMat)` #164

Closed devmotion closed 2 years ago

devmotion commented 2 years ago

This PR simplifies kron with PDiagMat and also avoids indexing issues (see #163).

On the master branch

julia> using PDMats, BenchmarkTools

julia> A = PDiagMat(rand(100));

julia> B = PDiagMat(rand(100));

julia> @btime kron($A, $B);
  13.395 μs (103 allocations: 166.55 KiB)

This PR

julia> using PDMats, BenchmarkTools

julia> A = PDiagMat(rand(100));

julia> B = PDiagMat(rand(100));

julia> @btime kron($A, $B);
  4.625 μs (6 allocations: 78.34 KiB)

Accidentally, I had pushed the changes directly to the master branch (already reverted it).

Edit: Additionally, the PR fixes some type inference issues:

On the master branch

julia> using PDMats, StaticArrays, Test

julia> typeof(kron(PDiagMat(@SVector([0.1, 0.2, 0.5])), PDiagMat(@SVector([0.2, 0.4, 1.2]))))
PDiagMat{Float64, SVector{9, Float64}}

julia> @inferred(kron(PDiagMat(@SVector([0.1, 0.2, 0.5])), PDiagMat(@SVector([0.2, 0.4, 1.2]))));
ERROR: return type PDiagMat{Float64, SVector{9, Float64}} does not match inferred return type PDiagMat
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] top-level scope
   @ REPL[9]:1

With this PR

julia> using PDMats, StaticArrays, Test

julia> typeof(kron(PDiagMat(@SVector([0.1, 0.2, 0.5])), PDiagMat(@SVector([0.2, 0.4, 1.2]))))
PDiagMat{Float64, SVector{9, Float64}}

julia> @inferred(kron(PDiagMat(@SVector([0.1, 0.2, 0.5])), PDiagMat(@SVector([0.2, 0.4, 1.2]))));
codecov-commenter commented 2 years ago

Codecov Report

Merging #164 (fb4a0ef) into master (da9fe95) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #164   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files           8        8           
  Lines         440      440           
=======================================
  Hits          395      395           
  Misses         45       45           
Impacted Files Coverage Δ
src/pdiagmat.jl 99.03% <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 da9fe95...fb4a0ef. Read the comment docs.

matbesancon commented 2 years ago

LGTM