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

Make Bernoulli produce Bools #1079

Merged
merged 3 commits into from
Mar 17, 2020
Merged

Conversation

MikeInnes
Copy link
Contributor

This effectively reverts #1048 and also addresses the original issue #984.

I'm not that familiar with this codebase, but it appears that rand(D, ...) uses the eltype of the distribution to create the right array container. For Bernoulli, eltype hit a fallback method that incorrectly returned Int64, so changing that to Bool fixes the issue.

@matbesancon
Copy link
Member

@MikeInnes
Copy link
Contributor Author

The cdf and ccdf functions were previously incorrect (but not tested since the Int rather than Bool versions got called).

Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this makes much more sense to use Bools here.

@codecov-io
Copy link

Codecov Report

Merging #1079 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
+ Coverage   80.62%   80.68%   +0.05%     
==========================================
  Files         113      113              
  Lines        5611     5612       +1     
==========================================
+ Hits         4524     4528       +4     
+ Misses       1087     1084       -3
Impacted Files Coverage Δ
src/univariate/discrete/bernoulli.jl 93.1% <100%> (+5.38%) ⬆️

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

@cossio
Copy link
Contributor

cossio commented Mar 4, 2020

After this PR, will rand(Bernoulli(0.5), 5,5) produce a BitArray?

@MikeInnes
Copy link
Contributor Author

@cossio not currently, since array generation just uses a fallback that creates an array based on the eltype. I'm sure that could be changed without too much difficulty later, but that should have its own discussion (I would be hesitant).

@matbesancon anything left to do for this to get merged and tagged?

@matbesancon
Copy link
Member

yes all good it seems, I wanted a last patch release before this one, which is now on the way

@matbesancon matbesancon merged commit 7a2f516 into JuliaStats:master Mar 17, 2020
matbesancon added a commit that referenced this pull request Mar 18, 2020
matbesancon added a commit that referenced this pull request Mar 18, 2020
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.

6 participants