support Float32 for alias sampling

[findmyway]

This fixes #158

[codecov]

Codecov Report

Merging #499 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
+ Coverage   83.99%   84.04%   +0.04%     
==========================================
  Files          21       21              
  Lines        2162     2162              
==========================================
+ Hits         1816     1817       +1     
+ Misses        346      345       -1

Impacted Files	Coverage Δ
src/sampling.jl	87.14% <100%> (ø)	⬆️
src/moments.jl	88.02% <0%> (+0.59%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad1fe0...27d8500. Read the comment docs.

[codecov]

Codecov Report

Merging #499 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
+ Coverage   83.99%   84.04%   +0.04%     
==========================================
  Files          21       21              
  Lines        2162     2162              
==========================================
+ Hits         1816     1817       +1     
+ Misses        346      345       -1

Impacted Files	Coverage Δ
src/sampling.jl	87.14% <100%> (ø)	⬆️
src/moments.jl	88.02% <0%> (+0.59%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad1fe0...27d8500. Read the comment docs.

[bkamins]

See #506 for a fix which uses AbstractWeights which I think is a bit cleaner.

(and bump this PR as I am also getting this error but with Int weights)

[findmyway]

Yeah, I added a limitation that the element type of w and a must be the same originally. If that's unnecessary, then I prefer your PR.

[bkamins]

The problem is that:

julia> using StatsBase

julia> Weights([1,2,3])
3-element Weights{Int64,Int64,Array{Int64,1}}:
 1
 2
 3

creates S and T as Int in your PR, while ap must be able to hold floats`.

The best way to make sure that all works is to add a test case with integer weights in your PR.

The best way to get a type for ap is to make it typeof(1*one(S)/one(T)). This should be inferred in compile time, but it would be best to make sure that this is what really happens 😄.

The other change is that in my PR we pass a weight vector to make_alias_table! which I think is enough as it already puts a restriction on its element types (via its constructor), but this is minor - we can leave it as in your PR (the type of a needs to change only, and probably it is enough that you make it a subtype of AbstractFloat in the signature).

[st--]

What's required of this PR before it can get merged? (related: JuliaStats/Distributions.jl#1074)

[bkamins]

AFAICT the implementation would have to be fixed and appropriate tests added to check its correctness.

[findmyway]

Sorry it's been a pretty long time. I can hardly remember what I tried to do here...

@bkamins Could you be more specific about what have to be fixed?

[bkamins]

As commented above ap cannot have T as its eltype because it will throw errors. You have to derive a proper eltype for it dynamically. I proposed the fomula typeof(1*one(S)/one(T)) and commented that all this should be carefully tested (as I have not tested this and only analyzed your proposal in my head).

[findmyway]

Ok, thanks, now I get it. It seems a bit more tricky than I thought. Let me do more local tests first.

[findmyway]

Could anyone help enable the CI?

@bkamins , I just notice that the rand function returns a Float64 by default. So I make it explicitly here:

https://github.com/JuliaStats/StatsBase.jl/pull/499/files#diff-5d02de7575ff9db21657bc7f703ab8b68543183487bf545784f37478c8ae6108R672

[findmyway]

Actually I think your original PR https://github.com/JuliaStats/StatsBase.jl/pull/506/files is enough. 😵😵😵

[bkamins]

Looks good. Let us wait for CI to pass

[devmotion]

Test failures with Julia nightly are unrelated, all other tests pass.

[devmotion]

Thank you for the PR @findmyway!