[randomness part 1] New utils/rand package for non-deterministic true randomness #4062
Conversation
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 2f1a1d0 The command Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||
tarakby
requested review from
ramtinms,
yhassanzadeh13,
AlexHentschel,
pattyshack,
Kay-Zee and
peterargue
| package math | ||
|
|
||
| // MinUint returns the minimum of a list of uints. | ||
| func MinUint(uints ...uint) uint { |
There was a problem hiding this comment.
Not used anywhere
| @@ -50,6 +50,15 @@ const ( | |||
| DefaultAddress = "localhost:0" | |||
| ) | |||
|
|
|||
| // returns a deterministic math/rand PRG that can be used for deterministic randomness in tests only. | |||
| // The PRG seed is logged in case the test iteration needs to be reproduced. | |||
| func GetPRG(t *testing.T) *rand.Rand { | |||
There was a problem hiding this comment.
will be used in subsequent PRs
utils/rand/rand.go
Outdated
| // and use big number modular reduction by `n`. | ||
| random := n | ||
| for random > max { | ||
| if _, err := rand.Read(buffer[:size]); err != nil { |
There was a problem hiding this comment.
you need to either check for numRead (aka _ in your code), or use io.ReadFull (same elsewhere)
There was a problem hiding this comment.
In the case of crypto/rand, checking one output is enough, as per the doc:
// Read is a helper function that calls Reader.Read using io.ReadFull.
// On return, n == len(b) if and only if err == nil.
I'll add a comment to clarify.
utils/rand/rand_test.go
Outdated
| } | ||
| stdev := stat.StdDev(distribution, nil) | ||
| mean := stat.Mean(distribution, nil) | ||
| assert.Greater(t, tolerance*mean, stdev, fmt.Sprintf("basic randomness test failed. stdev %v, mean %v", stdev, mean)) |
There was a problem hiding this comment.
similar to my comment in the other pr. stddev is meaningless in uniform distribution. may just check for expectedMean * (1 - tolerance) < mean < expectedMean * (1 + tolerance).
Also, maybe refactor the test cases to use the same skeleton (the only change that change is how bucket width/index is computed)
There was a problem hiding this comment.
I answered your comment here: #4067 (comment)
Good idea about the refactoring the test common logic.
|
bors merge |
Codecov Report
@@ Coverage Diff @@
## master #4062 +/- ##
==========================================
- Coverage 52.56% 51.20% -1.36%
==========================================
Files 772 672 -100
Lines 72985 60187 -12798
==========================================
- Hits 38365 30821 -7544
+ Misses 31503 26898 -4605
+ Partials 3117 2468 -649
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 225 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
( result of splitting #4052 into several PRs)
crypto/randto export some tools needed in the code base (Unitn, Shuffle, Sample..). This package provides the same useful APIs asmath/randand it should be used instead ofmath/randin production code.