Add NamedTupleVariate and ProductNamedTupleDistribution

[sethaxen]

Implements #1762

[sethaxen]

One challenge is that eltype for a distribution d doesn't tell us what type rand(d) will return. The worst case here is LKJCholesky, for which the eltype is Float64, but rand returns a Cholesky{Float64}. So without calling rand recursively on all distributions once, we cannot know the exact types a call to rand(::ProductNamedTupleDistribution) should return.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 98.36066% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.16%. Comparing base (b219803) to head (28a7c00).

Files with missing lines	Patch %	Lines
src/namedtuple/productnamedtuple.jl	98.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1803      +/-   ##
==========================================
+ Coverage   85.99%   86.16%   +0.17%     
==========================================
  Files         144      146       +2     
  Lines        8666     8727      +61     
==========================================
+ Hits         7452     7520      +68     
+ Misses       1214     1207       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[devmotion]

One challenge is that eltype for a distribution d doesn't tell us what type rand(d) will return. The worst case here is LKJCholesky, for which the eltype is Float64, but rand returns a Cholesky{Float64}. So without calling rand recursively on all distributions once, we cannot know the exact types a call to rand(::ProductNamedTupleDistribution) should return.

IMO eltype is a design flaw and should be removed completely. So I don't think it should block the PR.

[sethaxen]

IMO eltype is a design flaw and should be removed completely. So I don't think it should block the PR.

Without eltype, how would you recommend container eltype be determined for rand calls such as rand(d, 2, 3)? So far I've assumed this is what eltype should return, but then of course there's LKJCholesky, which returns Float64 when the container eltype is Cholesky{Float64}.

[devmotion]

No, that's not what eltype is supposed to give you currently. It is intended to return the element type of a single variate (e.g., Float64 for a MvNormal whose variates are Vector{Float64}). But IMO there are at least three major problems: 1) It's implemented inconsistently and everyone expects something different; 2) It's a misnomer (IMO it was a suboptimal decision in retrospect to re-use eltype in Random; the less known randtype seems much clearer); 3) The concept breaks down if you try to allow variates of dynamic types (depending on the return type of the rand(rng) calls and/or the type of the parameters). In my experience any heuristic for addressing 3) is doomed to fail in some cases, so my suggestion would be to call rand if you want to know the type of its return value (and if you're adventurous you could try to infer it with return_type but IMO that's too brittle and internal for a default definition in Distributions).

[sethaxen]

the less known randtype seems much clearer

In which package is randtype defined? I have not heard of it.

It is intended to return the element type of a single variate (e.g., Float64 for a MvNormal whose variates are Vector{Float64}).

Ah, okay, then this PR currently implements it incorrectly. Will fix.

my suggestion would be to call rand if you want to know the type of its return value

Cool, this is the approach I took, so will leave as is.

[devmotion]

In which package is randtype defined? I have not heard of it.

Sorry, I misremembered, it's called gentype. It's defined in Random, it also appeared in some discussion in some issue in Distributions, but IIRC there were PRs that tried to remove it, so not everyone shares my opinion 😄

Ref #1402 (comment)
Ref JuliaLang/julia#31968

[sethaxen]

I added a new API function marginal. While we have component for mixture distributions, we didn't have any API function for accessing marginals of a product distribution. And there are other cases where we know the marginals and users might want them (e.g. slices of MvNormal). It's an optional addition, and I'm fine with removing if maintainers think this should be in its own PR.

RE docs, currently product_distribution is documented in the multivariate distributions page, but with this PR, sometimes product_distribution will return a different variate form, so I think it makes sense to move all docs related to product distributions to their own page. marginal (if kept) could be documented here as well, though it's more general than that.

[sethaxen]

@devmotion this is ready for a review

[sethaxen]

@devmotion what do you think about this proposed docs restructure?

RE docs, currently product_distribution is documented in the multivariate distributions page, but with this PR, sometimes product_distribution will return a different variate form, so I think it makes sense to move all docs related to product distributions to their own page.

[sethaxen]

Bump @devmotion

[devmotion]

what do you think about this proposed docs restructure?

I think it's reasonable to move it.

[sethaxen]

@devmotion I restructured the docs. I also noticed that, while Product is deprecated, its docstring was missing a deprecation warning, so I added it. I also figured the behavior of mean(::ProductNamedTuple), mode, var, etc may not be obvious, so I added a doctested example.

[sethaxen]

Bump @devmotion

[sethaxen]

@devmotion alright, I've implemented the requested changes.

[devmotion]

Looks good to me, thank you 🙂