-
Notifications
You must be signed in to change notification settings - Fork 2
crrand: add Perm64 #21
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
Conversation
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.
Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions
crrand/perm.go line 21 at r1 (raw file):
import "math/bits" // MakePerm64 constructs a new Mixer from a 64-bit seed, providing a
[nit] constructs a Perm64
crrand/perm.go line 33 at r1 (raw file):
s1 := bits.RotateLeft64(seed^c0, 13) s2 := bits.RotateLeft64(seed^c1, 37) s3 := bits.RotateLeft64(seed^(c0^c1), 53)
[nit] unnecessary parens
crrand/perm.go line 49 at r1 (raw file):
// Nth returns the nth value in the permutation of the 64-bit values. The return // value may be passed to Index to recover n. The permutation is pseduorandom. func (p Perm64) Nth(n uint64) uint64 {
[nit] consider Apply() and Inverse() or maybe At() and IndexOf().
crrand/perm.go line 50 at r1 (raw file):
// value may be passed to Index to recover n. The permutation is pseduorandom. func (p Perm64) Nth(n uint64) uint64 { // Use a simple Feistel network with 4 rounds to shuffle data.
If this implementation is inspired from somewhere, can you cite the source?
crrand/perm.go line 51 at r1 (raw file):
func (p Perm64) Nth(n uint64) uint64 { // Use a simple Feistel network with 4 rounds to shuffle data.
[nit] extra blank line
crrand/perm.go line 76 at r1 (raw file):
// ARX-only round function. func f(x, k uint32) uint32 {
[nit] rename to arx
crrand/perm_test.go line 32 at r1 (raw file):
func TestPerm64(t *testing.T) { for _, seed := range interestingUint64s { mixer := MakePerm64(seed)
[nit] s/mixer/perm
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.
Reviewable status: 0 of 2 files reviewed, 10 unresolved discussions (waiting on @jbowens)
crrand/perm.go line 22 at r2 (raw file):
// MakePerm64 constructs a new Perm64 from a 64-bit seed, providing a // deterministic, pseduorandom, bijective mapping of 64-bit values X to 64-bit
s/pseduorandom/pseudorandom
crrand/perm.go line 42 at r2 (raw file):
} // A Perm64 provides a deterministic, pseduorandom permutation of 64-bit values.
s/pseduorandom/pseudorandom
crrand/perm.go line 48 at r2 (raw file):
// At returns the nth value in the permutation of the 64-bit values. The return // value may be passed to Index to recover n. The permutation is pseduorandom.
s/pseduorandom/pseudorandom
Add a type Perm64 that implements a psedurandom permutation of 64 bit integers.
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.
Reviewable status: 0 of 2 files reviewed, 12 unresolved discussions (waiting on @jbowens)
crrand/perm.go line 25 at r2 (raw file):
// values Y. func MakePerm64(seed uint64) Perm64 { // derive 4 x 32-bit round keys from the 64-bit seed using only ARX ops.
[nit] We're not using ARX here (no Addition).
crrand/perm.go line 35 at r2 (raw file):
s3 := bits.RotateLeft64(seed^c0^c1, 53) m.seed[0] = uint32(s0)
[nit] This looks a little strange.. we're only using 32-bits of each value but we're choosing either the low or the high?
|
Previously, RaduBerinde wrote…
We could just initialize a PCG and use |
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.
TFTR!
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @RaduBerinde)
crrand/perm.go line 21 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] constructs a Perm64
Done
crrand/perm.go line 49 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] consider
Apply()andInverse()or maybeAt()andIndexOf().
Done
crrand/perm.go line 51 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] extra blank line
Done
crrand/perm.go line 76 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] rename to
arx
Done
crrand/perm.go line 22 at r2 (raw file):
Previously, RaduBerinde wrote…
s/pseduorandom/pseudorandom
Done
crrand/perm.go line 25 at r2 (raw file):
Previously, RaduBerinde wrote…
We could just initialize a PCG and use
Uint32()on it four times instead of rolling out our own..
Good idea—done
crrand/perm.go line 35 at r2 (raw file):
Previously, RaduBerinde wrote…
[nit] This looks a little strange.. we're only using 32-bits of each value but we're choosing either the low or the high?
Done
crrand/perm.go line 42 at r2 (raw file):
Previously, RaduBerinde wrote…
s/pseduorandom/pseudorandom
Done
crrand/perm_test.go line 32 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] s/mixer/perm
Done
|
I don't think you pushed the latest changes? (or github has a hiccup) |
Add a type Perm64 that implements a pseudorandom permutation of 64 bit integers.
This change is