WIP: Add keyword constructors

[simonbyrne]

Fixes #768, fixes #977.

[ararslan]

I don't necessarily oppose this, but seeing it implemented, I kind of like it less than I thought I would.

[simonbyrne]

Good to speak up now. Why so?

[ararslan]

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.)

[simonbyrne]

Yeah, I concede it perhaps isn't as concise as I would have liked. Keyword aliases (simonbyrne/KeywordDispatch.jl#2) would help reduce some of the verbosity.

[simonbyrne]

That said, I do look forward to being able to write things like LogNormal(mean=..., sigma=...)

[nalimilan]

Why not have both positional and keyword argument versions?

[simonbyrne]

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).

[rofinn]

I think params should return a named tuple if this is the direction we want to go.

[simonbyrne]

I think params should return a named tuple if this is the direction we want to go.

Good idea.

[matbesancon]

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

[simonbyrne]

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

[EricForgy]

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.

[simonbyrne]

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)

[nalimilan]

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.

[matbesancon]

I think I had never dug that deep into the tests in JSON madness before :'(

[simonbyrne]

Yeah, I would love to rethink how we handle the tests. Not sure what the best options are.

[matbesancon]

these are weird because we write Julia code in a JSON, to then execute it, instead of having plain Julia files

[simonbyrne]

are you manually editing these, or were they generated from the R script?

[simonbyrne]

These are supposed to be generated by the gendref.R file.

[matbesancon]

ok, but the .lst is supposed to contain the Julia syntax correct?

[simonbyrne]

Honestly, I didn't try to figure it out.

[matbesancon]

I'm getting the R deps now to re-generate the JSON and see if all good

[matbesancon]

Manually editing, how can the R script edit those?

…

On Tue, Feb 26, 2019 at 9:04 PM Simon Byrne ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/ref/continuous_test.ref.json <#823 (comment)> : > @@ -879,7 +879,7 @@ ] }, { - "expr": "Erlang(3)", + "expr": "Erlang(alpha=3)", are you manually editing these, or were they generated from the R script? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#823 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHRRsip3OjAk1RjdjX8mKjjMuWAmS7yIks5vRZNpgaJpZM4aVB8Z> .

-- Mathieu Besançon

[matbesancon]

Error while running the R script

On Binomial() --> Binomial: n=1 p=0.5 
On Binomial(n=3) --> Binomial: n=NA p=0.5 
Error in if (vmax - vmin + 1 <= 10) { : 
  missing value where TRUE/FALSE needed

[simonbyrne]

I'm pretty short on time at the moment, so don't really time to spare on this: if someone wants to tackle the tests (or just leave them?), I'd be grateful.

[taqtiqa-mark]

It would be great to how that JuliaLang/julia/issues/9498 has no impact here, and for users employing this package. Not sure if this can be answered/assured via tests?

[simonbyrne]

It won’t be an issue in this case, since all the keyword based constructors have no positional arguments.

[taqtiqa-mark]

Disclaimer: I am no Julia guru....
IIUC, the issues are more likely to show up in cases where a general dist is implemented with default values (note to self, how to deal with ensuring tests of the automagic methods generated) and then other dist's are defined as special cases exploiting those defaults.

May be paranoid here..... but personally I'd avoid all this until Julia matures on a solution. Ack that right now such time appears to be Julia 2.0 - or whenever Julia dispatches on keyword arguments.

Even more heuristic reasoning: Julia's special sauce/advantage is multiple dispatch. Keyword arguments can't be dispatched on - the duck-test suggests keyword arguments aren't quite ready for prime time.

Also, wouldn't it be relatively simple to make a DistributionsKeywords.jl for those with use cases where someone can be more adventurous, or that just must have readable code: LogNormal(;mu=>1.0, sigma=>1.0)?

[maximerischard]

Named arguments would definitely be a big improvement in code readability. But KeywordDispatch seems like a fairly complex and fragile way to accomplish this, compared to a simpler solution using Base.@kwdef. In particular, I'm not enamored with enabling multiple parameter names and parametrizations via constructors, it feels too magical. When reading code, I would prefer to see σ=sqrt(3.0) rather than var=3.0. If nothing else d.σ works but d.var will not. The @kwdispatch sections for each distribution add boilerplate and opportunities for bugs, especially when implementing custom distributions.
With all that being said, I still think this PR is worth pursuing, it's a shame that work hasn't continued on it.

[ghost]

Are there still any plans to add this to Distributions.jl? I've definitely felt like this could be very useful before.

[seabbs]

Has there been any progress on this idea that isn't recorded here? It would be really great functionality to have - especially when trying to tempt new users over from other languages. Our specific use case (https://github.com/CDCgov/Rt-without-renewal/tree/main/EpiAware) is a case in point as aimed at epidemiologists who may not have the background needed to get the transforms right all the time without package support.

[andreasnoack]

I don't think there have been any developments and I completely agree that this would be very useful.

[devmotion]

I tried to start with a smaller PR a while ago because I thought it might be easier to move forward in smaller chunks: #1405 But there was no response so I did not push it further.

[seabbs]

Yes I saw that and it seemed like a nice simple solution to be getting on with.

But there was no response so I did not push it further.

I'm not clear how dev works around here or how to find out but this seems like a shame!