-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove constraint on AbstractPDMat #1552
base: master
Are you sure you want to change the base?
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
rand
is called)TODO
logpdf
rand
sqmahal(d::MvNormal, x::AbstractVector)
sqmahal(d::MvNormal, x::AbstractMatrix)
MvNormalCanon