-
Notifications
You must be signed in to change notification settings - Fork 481
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
SYSTEMDS-3556 Counter based random number generator #2186
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2186 +/- ##
============================================
+ Coverage 71.84% 71.87% +0.03%
- Complexity 44596 44731 +135
============================================
Files 1448 1453 +5
Lines 168967 169394 +427
Branches 32934 33012 +78
============================================
+ Hits 121393 121758 +365
- Misses 38244 38299 +55
- Partials 9330 9337 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many good things here, please address the minor comments, and describe the performance differences in the PR.
Next steps, is to integrate it into the compiler as options, i would suggest to do it via arguments to random "CB_uniform" could be the argument for instance.
src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixDatagen.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixDatagen.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sysds/runtime/matrix/data/RandomMatrixGenerator.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/sysds/test/component/matrix/LibMatrixDatagenTest.java
Show resolved
Hide resolved
src/test/java/org/apache/sysds/test/component/matrix/LibMatrixDatagenTest.java
Outdated
Show resolved
Hide resolved
If you resolve some of the comments, please feel free to mark them as such. |
Signed-off-by: chris-1187 <[email protected]>
if (valuePRNG instanceof PRNGenerator) { | ||
rngStream = Stream.generate(() -> min + (range * ((PRNGenerator) valuePRNG).nextDouble())).iterator(); | ||
} else if (valuePRNG instanceof CounterBasedPRNGenerator) { | ||
rngStream = Arrays.stream(((CounterBasedPRNGenerator)valuePRNG).getDoubles(ctr, blockrows * blockcols)).map(i -> min + (range * i)).iterator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup the stream looks nice, however, since it modifies old code, we need to be careful that the new code returns the same values as the previous.
Summary
Adding counter based RNG which improves random quality and speed
Details
The PRNG uses Philox4x64_10 to generate batches of random double values with a 64 bit randomness. The algorithm was tested for correctness by comparing its output for certain counters and keys with an existing implementation in c.
This reference implementation called openRAND was tested by the authors using various statistical methods described here.
Advantages