MeasureBase.jl icon indicating copy to clipboard operation
MeasureBase.jl copied to clipboard

`rand` in `ProductMeasure` incompatible with Distributions `rand()`

Open theogf opened this issue 4 years ago • 2 comments

I think because of the ::Type{T} in https://github.com/cscherrer/MeasureBase.jl/blob/917304dc4f13e8d353306d599ffc89b2f8b2acac/src/combinators/product.jl#L56 it is not possible to sample from Distributions.jl distributions.

theogf avatar Dec 15 '21 15:12 theogf

Thanks @theogf . The issue isn't exactly here, but in the call passed to each marginal. In master this is

function Base.rand(rng::AbstractRNG, ::Type{T}, d::ProductMeasure) where {T}
    _rand(rng, T, d, marginals(d))
end

function _rand(rng::AbstractRNG, ::Type{T}, d::ProductMeasure, mar) where {T}
    (rand(rng, T, m) for m in mar)
end

But I think this works better (for AbstractMeasures):

function Base.rand(rng::AbstractRNG, ::Type{T}, d::AbstractProductMeasure) where {T}
    map(marginals(d)) do dⱼ
        rand(rng, T, dⱼ)
    end
end

But in each case there's a call like rand(rng, T, dⱼ). @devmotion originally suggested this form -- the idea is that T<:AbstractFloat is passed down until it eventually gets to e.g. rand(T) -- but it hasn't made it into Distributions yet. David, is this in the works?

Until this is supported in Distributions, we can add a method to dispatch on products of Distributions. Let's leave this open until that's in place.

cscherrer avatar Dec 15 '21 15:12 cscherrer

but it hasn't made it into Distributions yet. David, is this in the works?

The plan is to use a different approach in Distributions: https://github.com/JuliaStats/Distributions.jl/pull/1433 (became a bit outdated due to a recent refactoring but it is mostly done) The PR adds support for rand([rng, ][::Type{T}, ]dist) but T will be the desired eltype of the samples (similar to e.g. rand(Float64, 2) or randn(Float32, 4)) but - usually - not be promoted in any way by the distribution. If no type is specified, a distribution (but - usually - not parameter) dependent element type is used; e.g., for continuous distributions usually it is Float64. You can read more about the motivation and the design in the PR. Mainly, I became more and more convinced that samples should - usually - not depend on the parameters (you can read more about it in the PR). Additionally, this approach seems simpler and more intuitive, and the PR has (almost) no breaking changes. Other maintainers also seem to support this suggestion.

devmotion avatar Dec 15 '21 20:12 devmotion