*(::Woodbury, ::Diagonal) is wrong

[AlexRobson]

This can be seen by:

julia> using PDMatsExtras, LinearAlgebra

julia> W = WoodburyPDMat(2 .* ones(4,2), Diagonal(5 * ones(2,)), Diagonal(3 * ones(4,)));

julia> D = Diagonal(rand(4,));

julia> M1 = D * W
4×4 WoodburyPDMat{Float64, Matrix{Float64}, Diagonal{Float64, Vector{Float64}}, Diagonal{Float64, Vector{Float64}}}:
 30.9114  33.6469  33.466   25.5906
 33.6469  42.3242  39.1597  29.9444
 33.466   39.1597  41.8703  29.7834
 25.5906  29.9444  29.7834  24.4827

julia> M2 = D * Matrix(W)
4×4 Matrix{Float64}:
 30.9114  28.7548  28.7548  28.7548
 39.3714  42.3242  39.3714  39.3714
 38.9491  38.9491  41.8703  38.9491
 22.7746  22.7746  22.7746  24.4827

Similar for

julia> M1 = W * D
4×4 WoodburyPDMat{Float64, Matrix{Float64}, Diagonal{Float64, Vector{Float64}}, Diagonal{Float64, Vector{Float64}}}:
 30.9114  33.6469  33.466   25.5906
 33.6469  42.3242  39.1597  29.9444
 33.466   39.1597  41.8703  29.7834
 25.5906  29.9444  29.7834  24.4827

julia> M2 = Matrix(W) * D
4×4 Matrix{Float64}:
 30.9114  39.3714  38.9491  22.7746
 28.7548  42.3242  38.9491  22.7746
 28.7548  39.3714  41.8703  22.7746
 28.7548  39.3714  38.9491  24.4827

The reasons the tests pass is because they are scale of the Identity matrix so they are commutative. e.g. here. Changing Diagonal to a random will cause these tests to fail.

My fault. I added these in originally

WIP PR: https://github.com/invenia/PDMatsExtras.jl/tree/ar/remove_diagonal

[mzgubic]

Oh, interesting, I only saw this now, sorry (not subscribed to all activity in PDMatsExtras). As far as I can tell the PR #21 both fixes this and adds ChainRules. Is it possible to disentangle the two? It would make the review better

[AlexRobson]

OK i'll look at that now. I already have an MR for it.

[AlexRobson]

#19

Just need to work out why codecov is complaining

[mzgubic]

codecov is weird sometimes, so no need to spend more than 5 mins