add method for Distributions.rand(::ResettableRNG, ::Binomial)

[slwu89]

This should address #248, which actually didn't have anything to do with product measure, but rather a method for the Binomial distribution. Also added a test to check for this behavior; it would fail previously.

[cscherrer]

Hi @slwu89 , looks like I have some catching up to do:

1. Welcome!
2. Sorry I had missed your Error when calling rand on a product measure #248
3. Thank you for the PR!!

I like the simplicity of your approach, but it specializes for Binomial, which shouldn't need to be the case. In this code

function Dists.rand(rng::ResettableRNG, d::Dists.Binomial)
    rand(rng.rng, d)
end

we'd usually instead write

function Dists.rand(rng::ResettableRNG, ::Type{T}, d::Dists.Binomial) where {T}
    rand(rng.rng, T, d)
end

The idea is that the T is propagated down to the low-level code, where it ends up calling rand(T), randexp(T), or randn(T). This works well in most cases, but for ResettableRNGs it's currently broken. I think at some point it's dispatching incorrectly. I'll have a look over the weekend; I'm guessing it can be a quick fix.

[slwu89]

Thanks @cscherrer! Ok, that all makes sense. I had a feeling that the specialization for that specific distribution wasn't necessary but I'm not fully familiar with how the internals are designed. And thanks for the welcome, I'm looking forward to getting more familiar with this package!

For what its worth the stack trace of my failure MWE (pasted at end of this comment) is below, it looks to me like the "crucial" part is around frame 4, where the call hits this line in Distributions.jl https://github.com/JuliaStats/Distributions.jl/blob/master/src/samplers/binomial.jl#L52

ERROR: MethodError: no method matching rng_native_52(::ResettableRNG{Xoshiro, Int64})
Closest candidates are:
  rng_native_52(::Xoshiro) at ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Random/src/Xoshiro.jl:75
  rng_native_52(::TaskLocalRNG) at ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Random/src/Xoshiro.jl:114
  rng_native_52(::RandomDevice) at ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Random/src/RNGs.jl:36
  ...
Stacktrace:
 [1] rand(r::ResettableRNG{Xoshiro, Int64}, #unused#::Random.SamplerTrivial{Random.UInt52Raw{UInt64}, UInt64})
   @ Random ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Random/src/generation.jl:114
 [2] rand(rng::ResettableRNG{Xoshiro, Int64}, X::Random.UInt52Raw{UInt64})
   @ Random ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Random/src/Random.jl:254
 [3] randexp(rng::ResettableRNG{Xoshiro, Int64})
   @ Random ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Random/src/normal.jl:118
 [4] rand(rng::ResettableRNG{Xoshiro, Int64}, s::Distributions.BinomialGeomSampler)
   @ Distributions ~/.julia/packages/Distributions/bQ6Gj/src/samplers/binomial.jl:52
 [5] rand(rng::ResettableRNG{Xoshiro, Int64}, d::Distributions.Binomial{Float64})
   @ Distributions ~/.julia/packages/Distributions/bQ6Gj/src/univariate/discrete/binomial.jl:139
 [6] rand
   @ ~/.julia/packages/MeasureTheory/y4m8h/src/parameterized/binomial.jl:49 [inlined]
 [7] rand
   @ ~/.julia/packages/MeasureTheory/y4m8h/src/combinators/product.jl:50 [inlined]
 [8] rand(rng::ResettableRNG{Xoshiro, Int64}, d::ProductMeasure{Vector{Binomial{(:n, :p), Tuple{Int64, Float64}}}})
   @ MeasureBase ~/.julia/packages/MeasureBase/1ryrV/src/rand.jl:7
 [9] top-level scope

MWE:

using Random
using MeasureTheory
rng = ResettableRNG(Random.Xoshiro(), 6542022242862247233)
mu = ProductMeasure([Binomial(n = 990, p = 0.00995017), Binomial(n = 10, p = 0.0246901)])
rand(rng, mu)

[github-actions]

Package name	latest	stable
Mitosis.jl
Soss.jl

[codecov]

Codecov Report

Base: 46.11% // Head: 45.98% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (9ff9bb5) compared to base (9960e9b).
Patch coverage: 33.33% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   46.11%   45.98%   -0.14%     
==========================================
  Files          43       43              
  Lines        1340     1344       +4     
==========================================
  Hits          618      618              
- Misses        722      726       +4     

Impacted Files	Coverage Δ
src/parameterized/binomial.jl	50.00% <0.00%> (ø)
src/resettable-rng.jl	43.24% <50.00%> (-5.25%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[github-actions]

Package name	latest	stable
Mitosis.jl
Soss.jl

[cscherrer]

Hi @slwu89 , I think I got the dispatch problems fixed. Here's your MWE now (using Julia 1.9, because that's just what I have loaded)

julia> using Random

julia> using MeasureTheory

julia> rng = ResettableRNG(Random.Xoshiro(), 6542022242862247233)
ResettableRNG(::Xoshiro, 6542022242862247233)

julia> mu = ProductMeasure([Binomial(n = 990, p = 0.00995017), Binomial(n = 10, p = 0.0246901)])
ProductMeasure([Binomial(n = 990, p = 0.00995017), Binomial(n = 10, p = 0.0246901)])

julia> rand(rng, mu)
2-element Vector{Int64}:
 12
  1

[cscherrer]

I think this is ready to go once tests pass. Thanks for catching this and starting on the fix! I changed the solution, but it was very helpful to have the test you added to make sure things are working properly.

[github-actions]

Package name	latest	stable
Mitosis.jl
Soss.jl

[slwu89]

Thanks @cscherrer! Great to see the "official" solution too, helps me understand better the design of the package.