Skip to content
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

MvNormal constructor is inconsistent between dim=1 and dim=2+ #1333

Closed
eperim opened this issue May 24, 2021 · 2 comments
Closed

MvNormal constructor is inconsistent between dim=1 and dim=2+ #1333

eperim opened this issue May 24, 2021 · 2 comments

Comments

@eperim
Copy link

eperim commented May 24, 2021

The MvNormal makes different assumptions depending on dim being 1 or more. For dim=1, it takes the argument as the std, while for dim=2+ it takes the argument as the cov:

julia> MvNormal(2.0 .* [1 0; 0 1])
ZeroMeanFullNormal(
dim: 2
μ: 2-element Zeros{Float64}
Σ: [2.0 0.0; 0.0 2.0]
)

julia> MvNormal(2.0 .* [1])
ZeroMeanDiagNormal(
dim: 1
μ: 1-element Zeros{Float64}
Σ: [4.0]
)

I understand that it is calling different constructors. In the second case, it assumes it's receiving a vector of stds:

julia> MvNormal(2.0 .* [1, 1])
ZeroMeanDiagNormal(
dim: 2
μ: 2-element Zeros{Float64}
Σ: [4.0 0.0; 0.0 4.0]
)

but this leads to a situation like this:

julia> A
1×1 Matrix{Any}:
 1
 
julia> MvNormal(2.0 .* A)
ZeroMeanFullNormal(
dim: 1
μ: 1-element Zeros{Float64}
Σ: [2.0]
)

julia> MvNormal(2.0 .* [1])
ZeroMeanDiagNormal(
dim: 1
μ: 1-element Zeros{Float64}
Σ: [4.0]
)

which seems dangerous. Probably having the constructor for the diagonal normal receive a vector of variances would make things more consistent and safer.

@st--
Copy link
Contributor

st-- commented Jul 7, 2021

I agree, having the vector case behave qualitatively differently from the matrix case is highly counter-intuitive, and an easy source of bugs.

@devmotion
Copy link
Member

IMO the constructors for distributions are generally a bit confusing since they only use positional arguments. I think constructors with keyword arguments would be a lot clearer.

However, I disagree with the OP here, there is really no inconsistency here - vectors and matrices are fundamentally different types so it is fine to treat them differently. Moreover, vectors are treated in the same way as scalars and the second positional argument in the Normal constructor, they all indicate standard deviations. If you want to provide a vector of variances you can just provide a Diagonal matrix:

julia> MvNormal(Diagonal([2.0, 3.0]))
ZeroMeanDiagNormal(
dim: 2
μ: 2-element Zeros{Float64}
Σ: [2.0 0.0; 0.0 3.0]
)

I'll close this issue since it is a duplicate of #584 and #1203. The proposed fix for the incosistencies in #1203 (comment) was to deprecate the constructors with a scalar standard deviation and a vector and only accept matrices. I think this is the way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants