-
Notifications
You must be signed in to change notification settings - Fork 419
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
Fitting does not respect type parameters #1544
Comments
It's known, the type only depends on the data. If you use |
Of course, the question is still if this should be changed. I think it would be reasonable. |
Thanks for your answer. Indeed, the behavior you describe seems logical too, since sufficient statistics are computed from the sample. |
Hum, after testing it doesn't quite go the way you said it would 😅 julia> using Distributions
julia> x, w = Float32[1, 2, 3], Float32[4, 5, 6];
julia> fit(Normal{Float32}, x)
Normal{Float64}(μ=2.0, σ=0.816496580927726)
julia> fit(Normal{Float32}, x, w)
ERROR: suffstats is not implemented for (Normal{Float32}, Vector{Float32}, Vector{Float32}).
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:33
[2] suffstats(::Type{Normal{Float32}}, ::Vector{Float32}, ::Vector{Float32})
@ Distributions ~/.julia/packages/Distributions/O4ZJg/src/genericfit.jl:5
[3] fit_mle(dt::Type{Normal{Float32}}, x::Vector{Float32}, w::Vector{Float32})
@ Distributions ~/.julia/packages/Distributions/O4ZJg/src/genericfit.jl:28
[4] fit(::Type{Normal{Float32}}, ::Vector{Float32}, ::Vector{Float32})
@ Distributions ~/.julia/packages/Distributions/O4ZJg/src/genericfit.jl:34
[5] top-level scope
@ REPL[62]:1 |
Well, it seems there are some hardcoded Float64 arguments, e.g. in https://github.com/JuliaStats/Distributions.jl/blob/master/src/univariate/continuous/normal.jl#L238. It would be easy to change these type restrictions at least in a first step. |
I think one would also need to make sufficient stats parametric, for instance here: Distributions.jl/src/univariate/continuous/normal.jl Lines 108 to 113 in dd6ae8f
|
yeah there are still a lot of F64 hardcoded sprinkled in the package :/ |
In case it helps resolve difficult design decisions, help?> fit_mle
search: fit_mle
fit_mle(D, x)
Fit a distribution of type D to a given data set x. From this, there's no doubt that the type parameters of |
I had given it a first shot months ago in this PR: Looking back at it, my main issue is that we need to accommodate both
|
This redesigns the constructors of `MvNormal` to: - check argument consistency in the inner constructor - support "coercion," for example `MvNormal{Float32}(μ, Σ)` will enforce that the result has eltype `Float32` - cascade coercion among the multiple type parameters The benefits of this design are subtle but significant, as it allows the user to specify intent, allows the constructors to accept input arguments that are not (yet) consistent with the desired output types (e.g., `MvNormal{Float64}(Any[1,2], I)`), and makes it easy for developers to "propagate" consistent types across the code base (e.g., fixes JuliaStats#1544).
I think something like #1823 would make it an easy fix, as the final line of any code that builds distributions just becomes function fit(::Type{D}, x) where D<:Normal
μ = mean(x)
σ = std(x)
return D(μ, σ)
end Just Works™. |
I think your attempt and mine fix different parts of the problem? |
Hi!
In my current project, I would like to fit distributions in reduced precision to speed up my code. I was therefore wondering if the following behavior is intentional:
The text was updated successfully, but these errors were encountered: