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

rand in ProductMeasure incompatible with Distributions rand() #34

Open
theogf opened this issue Dec 15, 2021 · 2 comments
Open

rand in ProductMeasure incompatible with Distributions rand() #34

theogf opened this issue Dec 15, 2021 · 2 comments

Comments

@theogf
Copy link
Collaborator

theogf commented Dec 15, 2021

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.

@cscherrer
Copy link
Collaborator

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.

@devmotion
Copy link
Member

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: JuliaStats/Distributions.jl#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.

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

No branches or pull requests

3 participants