Remove constraint on AbstractPDMat #1552
Conversation
|
I think it would be great if it would be possible to use different matrices and types as covariance matrices. I'm not happy about the added complexity in Distributions though and think there should be a better way to handle this. In particular, I think that similar generalizations are also desirable in other scenarios where currently Maybe we could instead add these fallbacks of |
|
I think that would make sense. One issue is I have no interest in implementing all of them, just like these 3, which is all we need for MvNormal. @andreasnoack what do you think. Would such a PR be welcomed to PDMats.jl? I would still want to add these test types here. One thing I have shown here right now is the code changes are not hard or deep. |
oxinabox
changed the title
|
This should now be good to go. |
|
I started the registration process for PDMats 0.11.11: JuliaRegistries/General#61420 Let's rerun tests when it's available. |
|
|
||
| function MvNormal(μ::AbstractVector{T}, Σ) where T | ||
| MvNormal{T, typeof(Σ), typeof(μ)}(μ, Σ) | ||
| end | ||
| function MvNormal(μ::AbstractVector{<:Real}, Σ::AbstractPDMat{<:Real}) |
There was a problem hiding this comment.
I guess here we might want to change it to
| function MvNormal(μ::AbstractVector{<:Real}, Σ::AbstractPDMat{<:Real}) | |
| function MvNormal(μ::AbstractVector{<:Real}, Σ::AbstractMatrix{<:Real}) |
There was a problem hiding this comment.
The tests pass, right now.
It took some effort to make sure the constructors cover the right cases and don't stackoverflow, or ambiguity error.
I could go and give them another pass over now that it is working, but I wouldn't want to just go and relax the ones there right now whily-nilly
There was a problem hiding this comment.
I think without this relaxation you can provoke a test error when you use something like MvNormal(::Vector{Float32}, ::BlockDiagonal{Float64}).
| μc = convert(AbstractArray{R}, μ) | ||
| Σc = convert(AbstractArray{R}, Σ) | ||
| MvNormal{R, typeof(Σc), typeof(μc)}(μc, Σc) |
There was a problem hiding this comment.
This change is not needed it seems, is it?
| μc = convert(AbstractArray{R}, μ) | |
| Σc = convert(AbstractArray{R}, Σ) | |
| MvNormal{R, typeof(Σc), typeof(μc)}(μc, Σc) | |
| MvNormal(convert(AbstractArray{R}, μ), convert(AbstractArray{R}, Σ)) |
There was a problem hiding this comment.
It is so we can call the parameterized constructor.
Otherwise we get a stackoverflow.
| end | ||
|
|
||
| function MvNormalCanon(μ::AbstractVector{T}, h::AbstractVector{T}, J::AbstractPDMat) where {T<:Real} | ||
| function MvNormalCanon(μ::AbstractVector{T}, h::AbstractVector{T}, J::P) where {T<:Real, P} |
There was a problem hiding this comment.
| function MvNormalCanon(μ::AbstractVector{T}, h::AbstractVector{T}, J::P) where {T<:Real, P} | |
| function MvNormalCanon(μ::AbstractVector{T}, h::AbstractVector{T}, J::AbstractMatrix{T}) where {T<:Real} |
| end | ||
| end | ||
|
|
||
| function MvNormalCanon(μ::AbstractVector{T}, h::AbstractVector{T}, J::AbstractPDMat) where {T<:Real} |
There was a problem hiding this comment.
I guess we want to keep this function but change J::AbstractPDMat to J::AbstractMatrix{<:Real}.
| R = promote_type(T, eltype(J)) | ||
| MvNormalCanon(convert(AbstractArray{R}, μ), convert(AbstractArray{R}, h), convert(AbstractArray{R}, J)) | ||
| MvNormalCanon{T,P,typeof(μ)}(convert(AbstractArray{R}, μ), convert(AbstractArray{R}, h), convert(AbstractArray{R}, J)) |
There was a problem hiding this comment.
For the old function here (relaxed to AbstractMatrix, see above):
| MvNormalCanon{T,P,typeof(μ)}(convert(AbstractArray{R}, μ), convert(AbstractArray{R}, h), convert(AbstractArray{R}, J)) | |
| MvNormalCanon(convert(AbstractArray{R}, μ), convert(AbstractArray{R}, h), convert(AbstractArray{R}, J)) |
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Closes #1219
Tests it works by using BlockDiagonals.jl
Basic philosophy:
randis called)TODO
logpdfrandsqmahal(d::MvNormal, x::AbstractVector)sqmahal(d::MvNormal, x::AbstractMatrix)MvNormalCanon