Distributions.jl
Distributions.jl copied to clipboard
JuliaStats
WIP: Add keyword constructors
I don't necessarily oppose this, but seeing it implemented, I kind of like it less than I thought I would.
It's hard to pinpoint, I guess it just seems a bit noisy, plus I worry that the names chosen for keyword arguments won't necessarily match all literature on a particular distribution, which will leave people confused. (Though I suppose positional arguments aren't necessarily better in that regard.)
Yeah, I concede it perhaps isn't as concise as I would have liked. Keyword aliases (https://github.com/simonbyrne/KeywordDispatch.jl/issues/2) would help reduce some of the verbosity.
That said, I do look forward to being able to write things like LogNormal(mean=..., sigma=...)
Why not have both positional and keyword argument versions?
It still does: I'm just deprecating some of what I think are ambiguous positional ones (e.g. single argument Normal constructor).
I think params should return a named tuple if this is the direction we want to go.
I think
paramsshould return a named tuple if this is the direction we want to go.
Good idea.
As discussed in the issue, I find the design better if we can keep error messages helpful, which is often the big point against macros
With this PR, if you use invalid keywords you'll get:
julia> Normal(a=1)
ERROR: KeywordMethodError: no keyword method matching Normal(; a::Int64)
Stacktrace:
[1] kwcall(::NamedTuple{(:a,),Tuple{Int64}}, ::Type) at /Users/simon/.julia/dev/KeywordDispatch/src/KeywordDispatch.jl:86
[2] #Normal#53(::Base.Iterators.Pairs{Symbol,Int64,Tuple{Symbol},NamedTuple{(:a,),Tuple{Int64}}}, ::Type) at ./none:0
[3] (::getfield(Core, Symbol("#kw#Type")))(::NamedTuple{(:a,),Tuple{Int64}}, ::Type{Normal}) at ./none:0
[4] top-level scope at none:0
I notice above the use of keywords mean and sigma.
Does that feel inconsistent? If you write out sigma, maybe mu is better. Or if you write out mean, maybe stddev or something is better.
I'd vote for mu and sigma.
No, that was intentional! If you specify mean or std parameters, it will match the moments.
julia> using Distributions
julia> LogNormal(mu=1.0, sigma=1.0)
LogNormal{Float64}(μ=1.0, σ=1.0)
julia> LogNormal(mean=1.0, sigma=1.0)
LogNormal{Float64}(μ=-0.5, σ=1.0)
julia> LogNormal(mean=1.0, std=1.0)
LogNormal{Float64}(μ=-0.34657359027997264, σ=0.8325546111576977)
The reason I used the lognormal is that in some cases you know the sigma variable, but want to match a particular mean (e.g. using geometric Brownian motion)
OK, sounds great overall! KeywordDispatch is really useful here.
I just wonder why Normal(x, y) should be allowed but not Normal(x). If we assume people know the order of arguments (reason why we keep the two-argument method), omitting the last argument doesn't sound confusing. OTC, since that's a standard pattern in Julia, it will likely annoy people not to be able to do that.
Also, some docstrings still need to be adjusted to the changes.
As for personal impressions, being able to use scale= and rate= will be great. We are keeping some positional constructors too.
I just wonder why
Normal(x, y)should be allowed but notNormal(x). If we assume people know the order of arguments (reason why we keep the two-argument method), omitting the last argument doesn't sound confusing. OTC, since that's a standard pattern in Julia, it will likely annoy people not to be able to do that.
It's not always the last argument that is omitted though, e.g. see MvNormal and VonMises.
It's not always the last argument that is omitted though, e.g. see
MvNormalandVonMises.
OK, then maybe only deprecate these?
I would prefer to get rid of them to be honest: I was hoping that the "standard" constructor would be the keyword ones, and the positional ones would mainly be for internal use.
That sounds quite radical to me. But then you'd have to define inner constructors to ensure they can't be used (since otherwise they are exported at the same time as the type itself).
We can decide that later. If people feel particularly strongly, I can add some of the constructors back for now.
We can decide to make constructors public later, but if don't add inner constructors to prevent people from using them now, they will de facto be part of the public API.
They're already part of the API. I'm just deprecating some I think are particularly confusing.
(unless I'm misunderstanding what you're saying)
I was referring to this:
I was hoping that the "standard" constructor would be the keyword ones, and the positional ones would mainly be for internal use.
There's no such thing as "constructors for internal use" in Julia: as long as the type is exported, the constructor also is.
So I've made a few changes to KeywordDispatch.jl to make this a bit simpler. I've updated the normal distribution with the new format, and updated the help text: https://github.com/JuliaStats/Distributions.jl/pull/823/files#diff-490e3dc074dbfc901fe90e4163ed3048
I would appreciate any reviews and comments now before I put the effort in updating the rest of the distributions accordingly.
Hmm, it is slightly worrying that CI is timing out. I'll need to figure out what is going on.
Okay, that's better. Other than some tests are there any other changes people want to see?
is the whole branch longer to compile than master? (question following your previous comment)
@matbesancon are you referring to the CI timing out? It was because I was still calling a deprecated method in a loop. This has now been fixed.
Ok that's perfect, I thought the kw dispatch mechanism caused too long compilation time. All good!
On Tue, Feb 26, 2019, 18:02 Simon Byrne [email protected] wrote:
@matbesancon https://github.com/matbesancon are you referring to the CI timing out? It was because I was still calling a deprecated method in a loop. This has now been fixed.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuliaStats/Distributions.jl/pull/823#issuecomment-467523177, or mute the thread https://github.com/notifications/unsubscribe-auth/AHRRsqDPDBtsbkj44TUT8LZdbBvS3Hp4ks5vRWidgaJpZM4aVB8Z .