Fixes to adddiag

[olivierverdier]

No description provided.

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

Files	Coverage Δ
src/addition.jl	50.00% <100.00%> (+4.76%)	⬆️
src/utils.jl	91.66% <100.00%> (ø)

📢 Thoughts on this report? Let us know!.

[devmotion]

Can you add a test with an example that would be fixed by the PR? In its current form it's difficult to tell what the PR improves.

Generally, we would like to use _adddiag! here since _adddiag would lead to additional undesired allocations.

[olivierverdier]

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?

[devmotion]

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?

[devmotion]

Thank you! I added an additional fix and tests in d10d1a7. I'll merge and release when tests pass.