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

[WIP] RNG test fixes and improvements #513

Merged
merged 23 commits into from
Mar 8, 2022

Conversation

vinaydes
Copy link
Contributor

@vinaydes vinaydes commented Feb 17, 2022

@MatthiasKohl @teju85 Please take a look.

@vinaydes
Copy link
Contributor Author

I'll take a look at failing test Rng.MeanError.

@vinaydes
Copy link
Contributor Author

vinaydes commented Mar 2, 2022

Summary of latest 3 commits:

  1. Make uniform generation same for both PCG and cuRAND - [0.0, 1.0)
  2. Change seed of GEMM unit test since it was generating too small floating point values to compare correctly
  3. Fix Inf/NaN occurrences by adding 0.0 checks at multiple places

@cjnolet cjnolet added the improvement Improvement / enhancement to an existing function label Mar 3, 2022
@cjnolet cjnolet added the non-breaking Non-breaking change label Mar 3, 2022
Copy link
Contributor

@MatthiasKohl MatthiasKohl left a comment

Choose a reason for hiding this comment

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

I went over all distributions again (not just the ones you changed). Things I noticed:

  1. For Bernoulli / ScaledBernoulli, I'm almost sure the calculation is simply wrong: the probability parameter usually represents chance of success, but with res > params.prob this would mean that for a probability of 40% we get success 60% of the time. So I guess here probability represents non-success. But, if params.prob is 0, we can still get non-success if res == 0. So I'm almost certain that it should actually be res < params.prob: For params.prob == 1, we always get success, for params.prob == 0, we always get non-success and everything in-between should work fine.
  2. In Laplace, there is something similar to the above, but it is correct in the code: I would still add a comment that since res is in [1, 2^24 - 1] / 2^24, we will have 2^23 numbers in the first branch and 2^23 - 1 numbers in the second one. The special value 2^23 / 2^24 == oneHalf would give the same result anyway in both branches, so it doesn't matter where it goes. This is probably not obvious at first sight for someone going over this code.

Apart from that, everything else looks good to me.

Copy link
Contributor

@MatthiasKohl MatthiasKohl left a comment

Choose a reason for hiding this comment

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

Changes LGTM from my side

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM

@cjnolet
Copy link
Member

cjnolet commented Mar 8, 2022

@gpucibot merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants