WIP / DNM: native counter-based random number generators - #182
WIP / DNM: native counter-based random number generators#182rileyjmurray wants to merge 25 commits into
Conversation
rileyjmurray
left a comment
There was a problem hiding this comment.
Various notes:
The underlying Engine's generate function shouldn't return a separate output array, it should write to a provided input array.
We should include at least one modern (post Random123) CBRNG in the this PR. It's worth doing that at as part of the monolithic change since that might affect API design.
We'll need implementation DevNotes before the merge.
The plan (not kept in tree long term) should indicate how our base random number generators compare to the tooling in the C++ STL.
Before starting serious implementation we need to discuss what C++ idioms might be used and why.
|
Ping @vasilisge0 |
602749d to
25f0cf7
Compare
|
@vasilisge0, I'm having codex start implementation work now. |
rileyjmurray
left a comment
There was a problem hiding this comment.
Here are comments that apply across multiple files.
- The copyright statement for Random123-derived files needs amending.
- The PR description (when written) should flag parts of the code that would benefit from machine-specific instructions or compiler pragmas for more up-to-date compilers.
- We need to talk about the use of the
classandprivatekeywords in new data structures. - Don't be so paranoid about line length. Screens are big these days.
- We need to acknowledge the role of CBRNGStream. It's weird that it's only in sparse_data.hh. (I remember putting it here, so it's my fault, but it's nevertheless weird.)
- I'd like to change templating expressions like
RandBLAS::CounterBasedRNGState State = RandBLAS::DefaultRNGStatetoCounterBasedRNGState state_t = DefaultRNGState; the in-libraryRandBLAS::qualification shouldn't be necessary. Separately, I prefer that the state type be referred to asstate_trather thanState. - Let's just rename
RandBLAS::CounterBasedRNGStatetoRandBLAS::RNGState. All of our RNGs will (always) be counter-based. (Please amend the comments above appropriately given this expression of preference.)
|
idea: have counter arrays support an |
This PR is a work in progress.
Summary
RNGState<Engine>adapter and structuralGeneratorStatecustomization boundary;RepackedOutputfor power-of-two output-word subdivision; andThe default
Philox<4, 32, 10>integer stream and default sparse-sketch output remain bitwise compatible with the previous Random123-backed implementation. Dense transforms retain the same formulas subject to host math-library rounding.Validation
The branch includes Philox known-answer tests, counter/repacking/transform unit tests, statistical tests, sampler regression tests, installed downstream and example builds, and clean builds with Random123 discovery disabled.
Deferred optimization opportunities
This PR uses portable implementations and intentionally defers architecture/compiler-specific tuning. Follow-up performance work could evaluate:
mulhilopath;These changes should be benchmarked by compiler and architecture before they replace the portable code.