Remove max sample value #1374
Remove max sample value #1374
Conversation
utils/sampler/uniform_replacer.go
Outdated
| @@ -82,7 +76,7 @@ func (s *uniformReplacer) Next() (uint64, error) { | |||
| return 0, errOutOfRange | |||
| } | |||
|
|
|||
| draw := uint64(s.rng.Int63n(int64(s.length-s.drawsCount))) + s.drawsCount | |||
| draw := s.rng.Uint64n(s.length-1-s.drawsCount) + s.drawsCount | |||
There was a problem hiding this comment.
Note the -1 here now because Uint64n returns values in the range [0, n] whereas Int63n returned values in the range [0, n)
There was a problem hiding this comment.
Would it be simpler to just have Uint64n return a number in [0, n)?
There was a problem hiding this comment.
Wondering if we should distinguish between these 2 random modes? Seems confusing that *n function now behaves differently (especially if it is different than the golang rand pkg: https://pkg.go.dev/math/rand#Int31n)?
There was a problem hiding this comment.
Would it be simpler to just have Uint64n return a number in [0, n)?
This would mean that we would need to panic if n == 0 which I was trying to avoid.
There was a problem hiding this comment.
Renamed Uint64n to Uint64Inclusive to be very clear
utils/sampler/uniform_resample.go
Outdated
| @@ -70,7 +64,7 @@ func (s *uniformResample) Next() (uint64, error) { | |||
| } | |||
|
|
|||
| for { | |||
| draw := uint64(s.rng.Int63n(int64(s.length))) | |||
| draw := s.rng.Uint64n(s.length - 1) | |||
There was a problem hiding this comment.
Note the -1 here now because Uint64n returns values in the range [0, n] whereas Int63n returned values in the range [0, n)
utils/sampler/weighted_best.go
Outdated
| samples := []uint64(nil) | ||
| if totalWeight > 0 { | ||
| samples = make([]uint64, s.benchmarkIterations) | ||
| for i := range samples { | ||
| samples[i] = uint64(globalRNG.Int63n(int64(totalWeight))) | ||
| samples[i] = globalRNG.Uint64n(totalWeight - 1) |
There was a problem hiding this comment.
Note the -1 here now because Uint64n returns values in the range [0, n] whereas Int63n returned values in the range [0, n)
utils/sampler/uniform_replacer.go
Outdated
| @@ -82,7 +76,7 @@ func (s *uniformReplacer) Next() (uint64, error) { | |||
| return 0, errOutOfRange | |||
| } | |||
|
|
|||
| draw := uint64(s.rng.Int63n(int64(s.length-s.drawsCount))) + s.drawsCount | |||
| draw := s.rng.Uint64n(s.length-1-s.drawsCount) + s.drawsCount | |||
There was a problem hiding this comment.
Would it be simpler to just have Uint64n return a number in [0, n)?
| s.rng = globalRNG | ||
| s.seededRNG = newRNG() | ||
| s.length = length | ||
| s.drawn = make(defaultMap) | ||
| s.drawsCount = 0 | ||
| return nil | ||
| } | ||
|
|
||
| func (s *uniformReplacer) Sample(count int) ([]uint64, error) { |
There was a problem hiding this comment.
This will panic if count = -1
s := uniformReplacer{}
s.Initialize(1)
s.Sample(-1)
There was a problem hiding this comment.
I feel like panicking is pretty reasonable when requesting a negative number of values
Why this should be merged
How this works
Extends the rng function to support generating all possible values in the range [0, MaxUint64]
How this was tested
n < MaxInt64or else this could result in an unexpected change in the proposervm calculation.