JuliaStats / PDMats.jl

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

Add FillArrays extension & bump Julia compat to >=1.6 #184

Closed devmotion closed 10 months ago

devmotion commented 11 months ago

This PR adds FillArrays as a weak dependency on Julia >= 1.9 and as a proper dependency on Julia < 1.9 (the latter might not be completely desirable, see https://github.com/JuliaStats/PDMats.jl/pull/182#issuecomment-1736061820, and alternatives would be to use Requires or reduce features on older Julia versions).

Currently, the only method in the extension is a definition of AbstractPDMat for Diagonal matrices with an FillArray as diagonal. This is motivated by the constructors of MvNormal in Distributions in https://github.com/JuliaStats/Distributions.jl/blob/87aebc29b2b9608801b70aae09fbc1d2dad56e3f/src/multivariate/mvnormal.jl#L201-L210: With the definition in this PR they could be simplified to

MvNormal(μ::AbstractVector{<:Real}, Σ::AbstractMatrix{<:Real}) = MvNormal(μ, AbstractPDMat(Σ))
MvNormal(μ::AbstractVector{<:Real}, Σ::UniformScaling{<:Real}) =
    MvNormal(μ, ScalMat(length(μ), Σ.λ))

This pattern is more generally useful, I imagine: Downstream packages that deal with pd matrices could be encouraged to use AbstractPDMat(...) for converting user-provided matrices to an optimized representation. The PR here adds such optimizations also for Diagonal with FillArrays as diagonal.

Edit: I dropped support for Julia < 1.6 in the PR. Tests were failing on Julia 1.0, even after adding compat with FillArrays 0.x - IIRC Pkg does not resolve again the package dependencies in the tests which caused issues due to incompatibilities between FillArrays and BandedMatrices.

codecov-commenter commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Files Coverage Δ
ext/PDMatsFillArraysExt.jl 100.00% <100.00%> (ø)
src/PDMats.jl 100.00% <ø> (ø)
src/scalmat.jl 97.33% <100.00%> (ø)
src/utils.jl 94.73% <ø> (+3.07%) :arrow_up:

:loudspeaker: Thoughts on this report? Let us know!.

devmotion commented 10 months ago

Closed in favour of #192.