Can one of the admins verify this patch?

add to allowlist

Not a comment, but a question @vinaydes
I have a case where I want to generate uniformly distributed bit-vectors, which are stored in an array of uint32_t. Can I use the next_u32 methods found in the rng_impl.cuh to accomplish this?

@teju85 If you want each bit to have 50% chance of being either 1 or 0, then next_u32 (or overloaded next with uint32_t data type) is correct.

Yes, that's correct. I'd like to have them 1/0 with 50% probability. Thanks for the confirmation.

I have tried to summarize status of all the action items came up with regard to this PR. Some of the items have to deferred in order to get this PR merged sooner.

From Matthias

1. Getter/setter methods for Rng class
   • Not done. We now have introduced RngState, do we still need getter/setter methods? Perhaps to RngState?
2. Introduce RngState
   • Done
3. Change the name for _base_subsequence
   • Done. Renamed to subsequence and is inside RngState
4. Improvements to advance method
   • Done. See advance() in rng_impl.cuh
5. Add default values for stride and idx
   • Done
6. Address discrepancy between Philox and PCG next_float() call. Details
   • WIP I have modified the code of PCG next_float and next_double to produce float in range (0, 1]. However bunch of tests are failing after this change. Once I fix the issue, I'll push the changes
7. For cpp/test/random/rng.cu: The comments mention sigma being multiplied by 4 (line 101) or 3-4 (line 185), but then it actually seems to multiply by 10 (line 102 and 244-245, 289-290). Is this because we actually have the variance instead of standard deviation? But in this case, why is it done with both mean and variance values?
   • Done/Deferred. The test seems to work even with num_sigma = 4, I had to change the seed for some of the tests to make them work. This will require some more work as I am not sure how correctness is checked here therefore I have added it to deferred items list.
8. Add swift method
   • Deferred

From Corey

9. Open corresponding cuML PR : [WIP] Changes required in cuML corresponding to RAFT RNG improvements cuml#4562
   • Done. The cuML PR is currently failing at CI, I am debugging it.
10. Are we planning to work the rng_state class into this PR? If not, it would be worth capturing some of the follow-on changes and discussion in an issue just so they don't get lost.
    • Done. RngState is introduced and deferred items are being tracked here
11. Update copyright year
    • Done
12. Add constructor for distribution parameter classes.
    • Deferred
13. Introduce rng_common.hpp for things that are common to random and detail namespace. Details
    • Deferred
14. Move box Muller transform to device specific namespace
    • Deferred

Bonus

1. I came across a bug in eigen value/vector test, which should be fixed with this PR.

@MatthiasKohl @cjnolet Let me know if I have missed any point.

@vinaydes Thank you very much for the detailed overview of items above!

A few minor comments from my side, all of which can be deferred to another PR:

1. We can probably remove offset from the RngState struct. The point of the struct is that it allows to abstract away how exactly we access/create the Rng class in the device function. Right now, if we call something like Rng::normal many times in a row, offset will never be updated and always be 0. Thus, I don't see a reason why we would should spend the additional constant/register memory and send the parameter from host to device. If someone wants to use all three parameters, they can still construct the device classes using the other constructor, like you do in fillKernel.
2. The constructors for the device classes using RngState should accept a thread-based offset. This is different to the offset in the RngState class because that thread-based offset must be added to the state in a way consistent with advance later on the host (see for example how you do it in fillKernel). Otherwise, users still have to know how the internals of the state work, i.e. adding the thread-based offset to subsequence, then updating the subsequence in advance by how many we had, and they must not forget about it!

@cjnolet can we get a final review and merge this PR soon, please? cuML RF and cugraph-ops sampling are blocked on this.

@teju85 this PR looks fine to go in but it is blocked by rapidsai/cuml#4562 since it's a breaking change on the cuml side.

Ah got it. Thanks Corey. That PR still seems to have build issues @vinaydes ?

Ah got it. Thanks Corey. That PR still seems to have build issues @vinaydes ?

Working to fix it. It is a tiny change.

@vinaydes thw cuml build will be broken until this PR is merged: rapidsai/cuml#4548. It might help to merge that into your branch RNG branch for now.

Right, I saw the breaking changes and for now reverted merge of 22.04 locally. I'll use 4548 once I fix the build issue on older commit. Thanks.

@MatthiasKohl Addressed your comments with latest push. I am deferring the task of reconciliation of next_float / next_double since it will require some more work.

Thanks, looks good to me!

@gpucibot merge