Closed olivierverdier closed 11 months ago
Yes, but _adddiag!
is not implemented the way it is called (it needs a third parameter), so this line does not work (and was presumably never called nor tested?).
Here is a piece of code that shows the problem:
# PR version
my_add_1(a::PDiagMat, b::AbstractPDMat) = PDMat(PDMats._adddiag(Matrix(b), a.diag))
# alternative version
my_add_2(a::PDiagMat, b::AbstractPDMat) = PDMat(PDMats._adddiag!(Matrix(b), a.diag, 1))
# current version
my_add_3(a::PDiagMat, b::AbstractPDMat) = PDMat(PDMats._adddiag!(Matrix(b), a.diag))
a = PDiagMat([1, 2])
A = randn(2, 2)
M = PDMat(A * A')
my_add_1(a, M) # works (with _adddiag)
my_add_2(a, M) # works (with _adddiag!)
my_add_3(a, M) # fails
I guess your point is that one should use my_add_2
instead of my_add_1
?
Yes, my point is that we should rather use something like PDMat(PDMats._adddiag!(Matrix(b), a.diag, true))
.
Can you change the PR to that version and add a currently failing example to the tests?
Thank you! I added an additional fix and tests in https://github.com/JuliaStats/PDMats.jl/pull/186/commits/d10d1a767d0ffcc0ad931d028a5ec2321ec43a44. I'll merge and release when tests pass.
Codecov Report
All modified lines are covered by tests :white_check_mark:
50.00% <100.00%> (+4.76%)
91.66% <100.00%> (ø)
:loudspeaker: Thoughts on this report? Let us know!.