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

Update aliastable.jl #1074

Closed
wants to merge 1 commit into from
Closed

Conversation

githubtomtom
Copy link
Contributor

enable rand() from non-Float64 mixtures, e.g. MixtureModel{Univariate,Continuous,Normal,Float32}, now only Float64 element type is supported.
a related pull request is made in StatsBase.make_alias_table!().

enable `rand()` from non-Float64 mixtures, e.g. `MixtureModel{Univariate,Continuous,Normal,Float32}`, now only `Float64` element type is supported.
a related [pull request](JuliaStats/StatsBase.jl#559) is made in `StatsBase. make_alias_table!()`.
@codecov-io
Copy link

Codecov Report

Merging #1074 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1074   +/-   ##
=======================================
  Coverage   80.62%   80.62%           
=======================================
  Files         113      113           
  Lines        5611     5611           
=======================================
  Hits         4524     4524           
  Misses       1087     1087
Impacted Files Coverage Δ
src/samplers/aliastable.jl 84.61% <100%> (ø) ⬆️

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 d521695...c9b38c9. Read the comment docs.

@matbesancon
Copy link
Member

Thanks for the PR! Two points:
Should we wait for the PR in StatsBase to be merged?
Could you add some tests with non-Float64 mixtures to ensure everything is working fine?

@githubtomtom
Copy link
Contributor Author

Should we wait for the PR in StatsBase to be merged?

no response from StatsBase so far... it's my first time PR there ... do u know the expected turnaround time of them?

Could you add some tests with non-Float64 mixtures to ensure everything is working fine?

I tried, but failed. Because I could not modify AliasTable{S} to AliasTable{S, T} :(

@st--
Copy link
Contributor

st-- commented Oct 9, 2021

ping! is there anything holding back this PR? (other than perhaps the corresponding PR in StatsBase, JuliaStats/StatsBase.jl#559 JuliaStats/StatsBase.jl#499, also pinged that one...)

@devmotion
Copy link
Member

IIRC we have to wait for the StatsBase PR.

@devmotion
Copy link
Member

This also needs tests.

@devmotion
Copy link
Member

With StatsBase 0.33.12, sampling from mixture models with component weights of type Float32 works fine (it errors with StatsBase 0.33.11 and was fixed by JuliaStats/StatsBase.jl#499). E.g.,

julia> using Distributions

julia> rand(MixtureModel([Normal(), Normal()], [0.5f0, 0.5f0]), 100)
100-element Vector{Float64}:
...

I'll close this PR since the issue that it tries to solve seems to be fixed by the change in StatsBase.

@devmotion devmotion closed this Oct 16, 2021
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

Successfully merging this pull request may close these issues.

5 participants