search

JuliaStats / PDMats.jl

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

Add defaults for `isposdef` and `ishermitian` #123

Closed devmotion closed 2 years ago

devmotion commented 3 years ago

This PR adds generic definitions of LinearAlgebra.isposdef and LinearAlgebra.ishermitian. It also removes Base.eltype and Base.ndims definitions which are not needed anymore since AbstractPDMat{T} <: AbstractMatrix{T}.

Fixes https://github.com/JuliaStats/PDMats.jl/issues/121. Fixes https://github.com/JuliaStats/PDMats.jl/issues/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 for isposdef?

andreasnoack commented 3 years ago

maybe I should add at least the failing examples for isposdef?

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.

devmotion commented 3 years ago

I thought about the failing examples in https://github.com/JuliaStats/PDMats.jl/issues/121.

andreasnoack commented 3 years ago

Ah. Yes, it would probably be good to add them.

devmotion commented 3 years ago

Bump 🙂

palday commented 3 years ago

LGTM, @andreasnoack ?

andreasnoack commented 3 years ago

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.

devmotion commented 3 years ago

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 isposdef only for PDMat, PDiagMat and ScalMat but not AbstractPDMat? It would also avoid the incorrect default isposdef(::PSDMat) = true.