Add defaults for isposdef and ishermitian
#123
Conversation
Which are they? I think it would be good to add trivial tests. I'm not sure how to test anything non-trivial here since you can't construct the instances if the matrix isn't Hermitian and positive definite. |
|
I thought about the failing examples in #121. |
|
Ah. Yes, it would probably be good to add them. |
| @test isposdef(PDMat([1.0 0.0; 0.0 1.0])) | ||
| @test isposdef(PDiagMat([1.0, 1.0])) | ||
| @test isposdef(ScalMat(2, 3.0)) |
There was a problem hiding this comment.
Can you add the same trivial tests for ishermitian?
There was a problem hiding this comment.
Only isposdef was broken, so I only added tests for these failing examples. ishermitian works fine for PDMat, PDiagMat, and ScalMat already without this PR, it is just more efficient since it does not have to check all entries explicitly with this PR. So the actual change in this PR can't be tested.
|
Bump 🙂 |
|
LGTM, @andreasnoack ? |
|
What is the thinking about the case PDiagMat([0.0, 0.0])? I also thinking about the consequences of allowing the zero matrix for PDMat since I have usecase where it would be handy. |
|
My impression was that PDMats (so far) is not designed to work with positive semi-definite matrices, and therefore https://github.com/invenia/PDMatsExtras.jl was created. But maybe it would still be better to define |
This PR adds generic definitions of
LinearAlgebra.isposdefandLinearAlgebra.ishermitian. It also removesBase.eltypeandBase.ndimsdefinitions which are not needed anymore sinceAbstractPDMat{T} <: AbstractMatrix{T}.Fixes #121. Fixes #118.
I am not completely sure what's the best way to test these definitions (in particular
ishermitian) - maybe I should add at least the failing examples forisposdef?