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.

Sign up for GitHub

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

Jump to bottom

Stack overflow in MvNormal(::Vector{Real}, UniformScaling{Float64} #211

Open
penelopeysm opened this issue Nov 29, 2024 · 2 comments · May be fixed by #210
Open

Stack overflow in MvNormal(::Vector{Real}, UniformScaling{Float64} #211

penelopeysm opened this issue Nov 29, 2024 · 2 comments · May be fixed by #210

Comments

@penelopeysm
Copy link

penelopeysm commented Nov 29, 2024 •
edited
Loading 

Status `~/test/Project.toml`
  [31c24e10] Distributions v0.25.113
  [37e2e46d] LinearAlgebra v1.11.0

julia> using LinearAlgebra: I

julia> using Distributions: MvNormal

julia> MvNormal(Float64[1.0, 2.0], 0.5 * I)
IsoNormal(
dim: 2
μ: [1.0, 2.0]
Σ: [0.5 0.0; 0.0 0.5]
)

julia> MvNormal(Real[1.0, 2.0], 0.5 * I)
ERROR: StackOverflowError:

This comes from here

https://github.com/JuliaStats/Distributions.jl/blob/3de6038ef9b041ceaf649bc324d60ca65fcf09eb/src/multivariate/mvnormal.jl#L190-L193

The underlying issue is that convert(AbstractArray{Real}, ScalMat(2, 0.5), as defined in PDMats.jl

PDMats.jl/src/scalmat.jl

Line 11 in 592634b

Base.convert(::Type{ScalMat{T}}, a::ScalMat) where {T<:Real} = ScalMat(a.dim, T(a.value))
doesn't actually convert to Real.

julia> using PDMats: ScalMat

julia> sm = ScalMat(2, 0.5)
2×2 ScalMat{Float64}:
 0.5  0.0
 0.0  0.5

julia> convert(AbstractArray{Real}, sm)
2×2 ScalMat{Float64}:
 0.5  0.0
 0.0  0.5

I wasn't quite sure if this should be an upstream issue (i.e. whether this is intended behaviour on the part of PDMats.jl), because ScalMat only keeps a single number as the scale parameter and Julia doesn't convert scalars to Real. On the other hand one could specify the type parameter explicitly:

julia> MvNormal(Real[1.0, 2.0], ScalMat{Real}(2, 0.5))
MvNormal{Real, ScalMat{Real}, Vector{Real}}(
dim: 2
μ: Real[1.0, 2.0]
Σ: [0.5 0.0; 0.0 0.5]
)
@devmotion
Copy link
Member

devmotion commented Nov 29, 2024 •
edited
Loading

That's a bug in PDMats IMO. As documented, convert(T, x) should "convert x to a value of type T". Julia's type parameters are invariant, so ScalMat{Float64} is not a subtype of ScalMat{Real}. The scalar case is different: Since Float64 <: Real, convert(Real, 1.0) may (and indeed does) return a Float64.

@devmotion devmotion linked a pull request Nov 29, 2024 that will close this issue
Open
@devmotion devmotion transferred this issue from JuliaStats/Distributions.jl Nov 29, 2024
@penelopeysm
Copy link
Author

Thanks for the very quick fix:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix convert for ScalMat JuliaStats/PDMats.jl
2 participants
@devmotion @penelopeysm