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.

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

Add NamedTupleVariate and ProductNamedTupleDistribution #1803

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

sethaxen
Copy link
Contributor

Implements #1762

@sethaxen
Copy link
Contributor Author

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
Copy link

codecov-commenter commented Nov 23, 2023

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

devmotion commented Nov 23, 2023

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
Copy link
Contributor Author

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 sethaxen marked this pull request as ready for review November 23, 2023 21:50
@sethaxen
Copy link
Contributor Author

@devmotion this is ready for a review

src/common.jl Outdated Show resolved Hide resolved
src/common.jl Outdated Show resolved Hide resolved
src/namedtuple/productnamedtuple.jl Outdated Show resolved Hide resolved
src/namedtuple/productnamedtuple.jl Outdated Show resolved Hide resolved
src/namedtuple/productnamedtuple.jl Outdated Show resolved Hide resolved
src/namedtuple/productnamedtuple.jl Show resolved Hide resolved
@sethaxen
Copy link
Contributor Author

@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
Copy link
Contributor Author

Bump @devmotion

@devmotion
Copy link
Member

what do you think about this proposed docs restructure?

I think it's reasonable to move it.

@sethaxen
Copy link
Contributor Author

@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
Copy link
Contributor Author

sethaxen commented Sep 2, 2024

Bump @devmotion

src/namedtuple/productnamedtuple.jl Outdated Show resolved Hide resolved
src/namedtuple/productnamedtuple.jl Show resolved Hide resolved
src/namedtuple/productnamedtuple.jl Show resolved Hide resolved
@sethaxen
Copy link
Contributor Author

sethaxen commented Sep 5, 2024

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

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you 🙂

test/namedtuple/productnamedtuple.jl Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants