Open sethaxen opened 10 months ago
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.
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 :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
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}
.
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).
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 aMvNormal
whose variates areVector{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.
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 https://github.com/JuliaStats/Distributions.jl/issues/1402#issuecomment-975168414 Ref https://github.com/JuliaLang/julia/issues/31968
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.
@devmotion this is ready for a review
Hi all, would it be possible to get this moving? I think it would be really great to have this feature!
This is really just waiting on a review. @devmotion?
Hi @devmotion, could we make this happen?
@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, sometimesproduct_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.
Bump @devmotion
what do you think about this proposed docs restructure?
I think it's reasonable to move it.
@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.
Bump @devmotion
@devmotion alright, I've implemented the requested changes.
Implements #1762