Correct mixGamma oddness check. #124
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See mixGamma http://gee.cs.oswego.edu/dl/papers/oopsla14.pdf on page 13. Also see jane streets implementation for comparison https://github.com/janestreet/core_kernel/blob/master/src/splittable_random.ml#L87.
|
I see the same check in the F# version so that must be my fault while porting the code from the paper. I’m not 100% sure about the bit masks in the Haskell version, but happy to take the blame there as well 👍 |
moodmosaic
approved these changes
Oct 25, 2017
This was referenced Oct 25, 2017
|
@moodmosaic More jest then blame, so apologies, I was just wanting to give @jystic a hard time. Bugs happen. |
|
@markhibberd but thanks for catching that one! I've no idea how it slipped through 🤠 |
I've been racking my brain to figure out why I did this, as it's clearly not in the F# version. I was playing in GHCi and I think this is probably why I did it: I think it might make more sense to just disable that warning for this file though. |
|
👍 |
Closed
|
FWIW, the Java version has the opposite (the one we had originally): |
moodmosaic
added a commit
to moodmosaic/haskell-hedgehog
that referenced
this pull request
Apr 25, 2018
moodmosaic
added a commit
to moodmosaic/haskell-hedgehog
that referenced
this pull request
Apr 25, 2018
moodmosaic
added a commit
to moodmosaic/haskell-hedgehog
that referenced
this pull request
Apr 26, 2018
moodmosaic
added a commit
to moodmosaic/haskell-hedgehog
that referenced
this pull request
Apr 26, 2018
moodmosaic
added a commit
to moodmosaic/haskell-hedgehog
that referenced
this pull request
May 23, 2018
…tion See also: - http://www.pcg-random.org/posts/bugs-in-splitmix.html - hedgehogqa#191 - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/SplittableRandom.java In http://www.pcg-random.org/posts/bugs-in-splitmix.html, most of the output is horribly formatted. So I went on and formatted the important part of the output from that article, where it highlights the bug: >``` >Prelude System.Random.SplitMix> map mkSMGen > [ 0x61c8864680b583eb > , 0xf8364607e9c949bd > , 0x88e48f4fcc823718 > , 0x7f83ab8da2e71dd1 > , 0x7957d809e827ff4c > , 0xf8d059aee4c53639 > , 0x9cd9f015db4e58b7 > , 0xf4077b0dbebc73c0 > , 0x305cb877109d0686 > , 0x359e58eeafebd527 > , 0xbeb721c511b0da6d > , 0x86466fd0fcc363a6 > , 0xefee3e7b93db3075 > , 0x79629ee76aa83059 > , 0x05d507d05e785673 > , 0x76442b62dddf926c ] > > [ SMGen {_seed = 16269636050940713694, _gamma = 1} > , SMGen {_seed = 11331281847540010644, _gamma = 1} > , SMGen {_seed = 5619286150931196027 , _gamma = 1} > , SMGen {_seed = 10281499533683189089, _gamma = 1} > , SMGen {_seed = 17485363145453140727, _gamma = 7} > , SMGen {_seed = 17937441790592959953, _gamma = 7} > , SMGen {_seed = 15529190628949695919, _gamma = 7} > , SMGen {_seed = 6957216782316743206 , _gamma = 7} > , SMGen {_seed = 13022414457527836317, _gamma = 15} > , SMGen {_seed = 2127073997383595602 , _gamma = 15} > , SMGen {_seed = 7685868475082380828 , _gamma = 15} > , SMGen {_seed = 6396586367974718702 , _gamma = 15} > , SMGen {_seed = 13838712901582905874, _gamma = 31} > , SMGen {_seed = 15464227283579875280, _gamma = 31} > , SMGen {_seed = 17142043965827948305, _gamma = 31} > , SMGen {_seed = 10266498716653543189, _gamma = 31} ] >Prelude System.Random.SplitMix> >``` It compares just *one* implementation of SplitMix, [this](https://github.com/phadej/splitmix/blob/f6895c4137b14e4752b0575c178199201eb41810/src/System/Random/SplitMix.hs) one, at some particular point in time. (I can see that the author went and fixed it recently as shown from the diff-output [here](haskellari/splitmix@bd0692b) and [here](haskellari/splitmix@c6ba2f3), so this means that now *The Bug* section of http://www.pcg-random.org/posts/bugs-in-splitmix.html is outdated.) --- Here's the exact same test with *Hedgehog.Internal.Seed*: ``` λ :t from from :: GHC.Word.Word64 -> Seed λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 7046029254386353131 11400714819323198485 , Seed 17885559969949501885 11400714819323198485 , Seed 9864166656744503064 11400714819323198485 , Seed 9188376289577737681 11400714819323198485 , Seed 8743694738624347980 11400714819323198485 , Seed 17928928724259255865 11400714819323198485 , Seed 11302328716527294647 11400714819323198485 , Seed 17584158569056203712 11400714819323198485 , Seed 3484863033197266566 11400714819323198485 , Seed 3863623312507393319 11400714819323198485 , Seed 13742489918233434733 11400714819323198485 , Seed 9675543792836633510 11400714819323198485 , Seed 17288824720004427893 11400714819323198485 , Seed 8746728143070965849 11400714819323198485 , Seed 420250731748546163 11400714819323198485 , Seed 8521984098521027180 11400714819323198485 ] λ ``` This looks weird, as I'm getting the *exact same γ* through all the seeds. If I change Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..fe1a0be 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -98,11 +98,11 @@ random = -- | Create a 'Seed' using a 'Word64'. -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) λ ``` I can now reproduce the bug [*exactly* as shown in the article](http://www.pcg-random.org/posts/bugs-in-splitmix.html#the-bug): ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 1 , Seed 11409286845259996466 1 , Seed 1931727433621677744 1 , Seed 307741759840609752 1 , Seed 8606169619657412120 7 , Seed 13651108307767328632 7 , Seed 125750466559701114 7 , Seed 6781260234005250507 7 , Seed 15306535823716590088 15 , Seed 7344074043290227165 15 , Seed 9920554987610416076 15 , Seed 3341781972484278810 15 , Seed 12360157267739240775 31 , Seed 600595566262245170 31 , Seed 1471112649570176389 31 , Seed 8100917074368564322 31 ] λ ``` This is actually great so far, as it points out that we got Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) wrong and we can now 'fix' it according to the 'reference' source. (At this point, I can tell that the only reliable source is the one found in Oracle's JDK8 implementation.) --- And finally, If I go and change the glorious Hedgehog.Internal.Seed [`mixGamma`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L202) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214) (essentially, reverting hedgehogqa#124): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..5428927 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -100,7 +100,7 @@ random = -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) -- | A predefined gamma value's needed for initializing the "root" instances of -- 'Seed'. That is, instances not produced by splitting an already existing @@ -192,7 +192,7 @@ mixGamma x = y = mix64variant13 x .|. 1 n = popCount $ y `xor` (y `shiftR` 1) in - if n >= 24 then + if n < 24 then y `xor` 0xaaaaaaaaaaaaaaaa else y λ ``` It 'fixes' the gamma value: ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 12297829382473034411 , Seed 11409286845259996466 12297829382473034411 , Seed 1931727433621677744 12297829382473034411 , Seed 307741759840609752 12297829382473034411 , Seed 8606169619657412120 12297829382473034413 , Seed 13651108307767328632 12297829382473034413 , Seed 125750466559701114 12297829382473034413 , Seed 6781260234005250507 12297829382473034413 , Seed 15306535823716590088 12297829382473034405 , Seed 7344074043290227165 12297829382473034405 , Seed 9920554987610416076 12297829382473034405 , Seed 3341781972484278810 12297829382473034405 , Seed 12360157267739240775 12297829382473034421 , Seed 600595566262245170 12297829382473034421 , Seed 1471112649570176389 12297829382473034421 , Seed 8100917074368564322 12297829382473034421 ] λ ```
moodmosaic
added a commit
to moodmosaic/haskell-hedgehog
that referenced
this pull request
May 23, 2018
…tion See also: - http://www.pcg-random.org/posts/bugs-in-splitmix.html - hedgehogqa#191 - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/SplittableRandom.java In http://www.pcg-random.org/posts/bugs-in-splitmix.html, most of the output is horribly formatted. So I went on and formatted the important part of the output from that article, where it highlights the bug: >``` >Prelude System.Random.SplitMix> map mkSMGen > [ 0x61c8864680b583eb > , 0xf8364607e9c949bd > , 0x88e48f4fcc823718 > , 0x7f83ab8da2e71dd1 > , 0x7957d809e827ff4c > , 0xf8d059aee4c53639 > , 0x9cd9f015db4e58b7 > , 0xf4077b0dbebc73c0 > , 0x305cb877109d0686 > , 0x359e58eeafebd527 > , 0xbeb721c511b0da6d > , 0x86466fd0fcc363a6 > , 0xefee3e7b93db3075 > , 0x79629ee76aa83059 > , 0x05d507d05e785673 > , 0x76442b62dddf926c ] > > [ SMGen {_seed = 16269636050940713694, _gamma = 1} > , SMGen {_seed = 11331281847540010644, _gamma = 1} > , SMGen {_seed = 5619286150931196027 , _gamma = 1} > , SMGen {_seed = 10281499533683189089, _gamma = 1} > , SMGen {_seed = 17485363145453140727, _gamma = 7} > , SMGen {_seed = 17937441790592959953, _gamma = 7} > , SMGen {_seed = 15529190628949695919, _gamma = 7} > , SMGen {_seed = 6957216782316743206 , _gamma = 7} > , SMGen {_seed = 13022414457527836317, _gamma = 15} > , SMGen {_seed = 2127073997383595602 , _gamma = 15} > , SMGen {_seed = 7685868475082380828 , _gamma = 15} > , SMGen {_seed = 6396586367974718702 , _gamma = 15} > , SMGen {_seed = 13838712901582905874, _gamma = 31} > , SMGen {_seed = 15464227283579875280, _gamma = 31} > , SMGen {_seed = 17142043965827948305, _gamma = 31} > , SMGen {_seed = 10266498716653543189, _gamma = 31} ] >Prelude System.Random.SplitMix> >``` It compares just *one* implementation of SplitMix, [this](https://github.com/phadej/splitmix/blob/f6895c4137b14e4752b0575c178199201eb41810/src/System/Random/SplitMix.hs) one, at some particular point in time. (I can see that the author went and fixed it recently as shown from the diff-output [here](haskellari/splitmix@bd0692b) and [here](haskellari/splitmix@c6ba2f3), so this means that now *The Bug* section of http://www.pcg-random.org/posts/bugs-in-splitmix.html is outdated.) --- Here's the exact same test with *Hedgehog.Internal.Seed*: ``` λ :t from from :: GHC.Word.Word64 -> Seed λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 7046029254386353131 11400714819323198485 , Seed 17885559969949501885 11400714819323198485 , Seed 9864166656744503064 11400714819323198485 , Seed 9188376289577737681 11400714819323198485 , Seed 8743694738624347980 11400714819323198485 , Seed 17928928724259255865 11400714819323198485 , Seed 11302328716527294647 11400714819323198485 , Seed 17584158569056203712 11400714819323198485 , Seed 3484863033197266566 11400714819323198485 , Seed 3863623312507393319 11400714819323198485 , Seed 13742489918233434733 11400714819323198485 , Seed 9675543792836633510 11400714819323198485 , Seed 17288824720004427893 11400714819323198485 , Seed 8746728143070965849 11400714819323198485 , Seed 420250731748546163 11400714819323198485 , Seed 8521984098521027180 11400714819323198485 ] λ ``` This looks weird, as I'm getting the *exact same γ* through all the seeds. If I change Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..fe1a0be 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -98,11 +98,11 @@ random = -- | Create a 'Seed' using a 'Word64'. -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) λ ``` I can now reproduce the bug [*exactly* as shown in the article](http://www.pcg-random.org/posts/bugs-in-splitmix.html#the-bug): ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 1 , Seed 11409286845259996466 1 , Seed 1931727433621677744 1 , Seed 307741759840609752 1 , Seed 8606169619657412120 7 , Seed 13651108307767328632 7 , Seed 125750466559701114 7 , Seed 6781260234005250507 7 , Seed 15306535823716590088 15 , Seed 7344074043290227165 15 , Seed 9920554987610416076 15 , Seed 3341781972484278810 15 , Seed 12360157267739240775 31 , Seed 600595566262245170 31 , Seed 1471112649570176389 31 , Seed 8100917074368564322 31 ] λ ``` This is actually great so far, as it points out that we got Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) wrong and we can now 'fix' it according to the 'reference' source. (At this point, I can tell that the only reliable source is the one found in Oracle's JDK8 implementation.) --- And finally, If I go and change the glorious Hedgehog.Internal.Seed [`mixGamma`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L202) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214) (essentially, reverting hedgehogqa#124): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..5428927 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -100,7 +100,7 @@ random = -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) -- | A predefined gamma value's needed for initializing the "root" instances of -- 'Seed'. That is, instances not produced by splitting an already existing @@ -192,7 +192,7 @@ mixGamma x = y = mix64variant13 x .|. 1 n = popCount $ y `xor` (y `shiftR` 1) in - if n >= 24 then + if n < 24 then y `xor` 0xaaaaaaaaaaaaaaaa else y λ ``` It 'fixes' the gamma value: ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 12297829382473034411 , Seed 11409286845259996466 12297829382473034411 , Seed 1931727433621677744 12297829382473034411 , Seed 307741759840609752 12297829382473034411 , Seed 8606169619657412120 12297829382473034413 , Seed 13651108307767328632 12297829382473034413 , Seed 125750466559701114 12297829382473034413 , Seed 6781260234005250507 12297829382473034413 , Seed 15306535823716590088 12297829382473034405 , Seed 7344074043290227165 12297829382473034405 , Seed 9920554987610416076 12297829382473034405 , Seed 3341781972484278810 12297829382473034405 , Seed 12360157267739240775 12297829382473034421 , Seed 600595566262245170 12297829382473034421 , Seed 1471112649570176389 12297829382473034421 , Seed 8100917074368564322 12297829382473034421 ] λ ```
moodmosaic
added a commit
to moodmosaic/haskell-hedgehog
that referenced
this pull request
May 23, 2018
See also: - http://www.pcg-random.org/posts/bugs-in-splitmix.html - hedgehogqa#191 - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/SplittableRandom.java In http://www.pcg-random.org/posts/bugs-in-splitmix.html, most of the output is horribly formatted. So I went on and formatted the important part of the output from that article, where it highlights the bug: >``` >Prelude System.Random.SplitMix> map mkSMGen > [ 0x61c8864680b583eb > , 0xf8364607e9c949bd > , 0x88e48f4fcc823718 > , 0x7f83ab8da2e71dd1 > , 0x7957d809e827ff4c > , 0xf8d059aee4c53639 > , 0x9cd9f015db4e58b7 > , 0xf4077b0dbebc73c0 > , 0x305cb877109d0686 > , 0x359e58eeafebd527 > , 0xbeb721c511b0da6d > , 0x86466fd0fcc363a6 > , 0xefee3e7b93db3075 > , 0x79629ee76aa83059 > , 0x05d507d05e785673 > , 0x76442b62dddf926c ] > > [ SMGen {_seed = 16269636050940713694, _gamma = 1} > , SMGen {_seed = 11331281847540010644, _gamma = 1} > , SMGen {_seed = 5619286150931196027 , _gamma = 1} > , SMGen {_seed = 10281499533683189089, _gamma = 1} > , SMGen {_seed = 17485363145453140727, _gamma = 7} > , SMGen {_seed = 17937441790592959953, _gamma = 7} > , SMGen {_seed = 15529190628949695919, _gamma = 7} > , SMGen {_seed = 6957216782316743206 , _gamma = 7} > , SMGen {_seed = 13022414457527836317, _gamma = 15} > , SMGen {_seed = 2127073997383595602 , _gamma = 15} > , SMGen {_seed = 7685868475082380828 , _gamma = 15} > , SMGen {_seed = 6396586367974718702 , _gamma = 15} > , SMGen {_seed = 13838712901582905874, _gamma = 31} > , SMGen {_seed = 15464227283579875280, _gamma = 31} > , SMGen {_seed = 17142043965827948305, _gamma = 31} > , SMGen {_seed = 10266498716653543189, _gamma = 31} ] >Prelude System.Random.SplitMix> >``` It compares just *one* implementation of SplitMix, [this](https://github.com/phadej/splitmix/blob/f6895c4137b14e4752b0575c178199201eb41810/src/System/Random/SplitMix.hs) one, at some particular point in time. (I can see that the author went and fixed it recently as shown from the diff-output [here](haskellari/splitmix@bd0692b) and [here](haskellari/splitmix@c6ba2f3), so this means that now *The Bug* section of http://www.pcg-random.org/posts/bugs-in-splitmix.html is outdated.) --- Here's the exact same test with *Hedgehog.Internal.Seed*: ``` λ :t from from :: GHC.Word.Word64 -> Seed λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 7046029254386353131 11400714819323198485 , Seed 17885559969949501885 11400714819323198485 , Seed 9864166656744503064 11400714819323198485 , Seed 9188376289577737681 11400714819323198485 , Seed 8743694738624347980 11400714819323198485 , Seed 17928928724259255865 11400714819323198485 , Seed 11302328716527294647 11400714819323198485 , Seed 17584158569056203712 11400714819323198485 , Seed 3484863033197266566 11400714819323198485 , Seed 3863623312507393319 11400714819323198485 , Seed 13742489918233434733 11400714819323198485 , Seed 9675543792836633510 11400714819323198485 , Seed 17288824720004427893 11400714819323198485 , Seed 8746728143070965849 11400714819323198485 , Seed 420250731748546163 11400714819323198485 , Seed 8521984098521027180 11400714819323198485 ] λ ``` This looks weird, as I'm getting the *exact same γ* through all the seeds. If I change Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..fe1a0be 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -98,11 +98,11 @@ random = -- | Create a 'Seed' using a 'Word64'. -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) λ ``` I can now reproduce the bug [*exactly* as shown in the article](http://www.pcg-random.org/posts/bugs-in-splitmix.html#the-bug): ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 1 , Seed 11409286845259996466 1 , Seed 1931727433621677744 1 , Seed 307741759840609752 1 , Seed 8606169619657412120 7 , Seed 13651108307767328632 7 , Seed 125750466559701114 7 , Seed 6781260234005250507 7 , Seed 15306535823716590088 15 , Seed 7344074043290227165 15 , Seed 9920554987610416076 15 , Seed 3341781972484278810 15 , Seed 12360157267739240775 31 , Seed 600595566262245170 31 , Seed 1471112649570176389 31 , Seed 8100917074368564322 31 ] λ ``` This is actually great so far, as it points out that we got Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) wrong and we can now 'fix' it according to the 'reference' source. (At this point, I can tell that the only reliable source is the one found in Oracle's JDK8 implementation.) --- And finally, If I go and change the glorious Hedgehog.Internal.Seed [`mixGamma`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L202) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214) (essentially, reverting hedgehogqa#124): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..5428927 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -100,7 +100,7 @@ random = -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) -- | A predefined gamma value's needed for initializing the "root" instances of -- 'Seed'. That is, instances not produced by splitting an already existing @@ -192,7 +192,7 @@ mixGamma x = y = mix64variant13 x .|. 1 n = popCount $ y `xor` (y `shiftR` 1) in - if n >= 24 then + if n < 24 then y `xor` 0xaaaaaaaaaaaaaaaa else y λ ``` It 'fixes' the gamma value: ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 12297829382473034411 , Seed 11409286845259996466 12297829382473034411 , Seed 1931727433621677744 12297829382473034411 , Seed 307741759840609752 12297829382473034411 , Seed 8606169619657412120 12297829382473034413 , Seed 13651108307767328632 12297829382473034413 , Seed 125750466559701114 12297829382473034413 , Seed 6781260234005250507 12297829382473034413 , Seed 15306535823716590088 12297829382473034405 , Seed 7344074043290227165 12297829382473034405 , Seed 9920554987610416076 12297829382473034405 , Seed 3341781972484278810 12297829382473034405 , Seed 12360157267739240775 12297829382473034421 , Seed 600595566262245170 12297829382473034421 , Seed 1471112649570176389 12297829382473034421 , Seed 8100917074368564322 12297829382473034421 ] λ ```
erikd
pushed a commit
to erikd/haskell-hedgehog
that referenced
this pull request
Mar 2, 2020
See also: - http://www.pcg-random.org/posts/bugs-in-splitmix.html - hedgehogqa#191 - http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/SplittableRandom.java In http://www.pcg-random.org/posts/bugs-in-splitmix.html, most of the output is horribly formatted. So I went on and formatted the important part of the output from that article, where it highlights the bug: >``` >Prelude System.Random.SplitMix> map mkSMGen > [ 0x61c8864680b583eb > , 0xf8364607e9c949bd > , 0x88e48f4fcc823718 > , 0x7f83ab8da2e71dd1 > , 0x7957d809e827ff4c > , 0xf8d059aee4c53639 > , 0x9cd9f015db4e58b7 > , 0xf4077b0dbebc73c0 > , 0x305cb877109d0686 > , 0x359e58eeafebd527 > , 0xbeb721c511b0da6d > , 0x86466fd0fcc363a6 > , 0xefee3e7b93db3075 > , 0x79629ee76aa83059 > , 0x05d507d05e785673 > , 0x76442b62dddf926c ] > > [ SMGen {_seed = 16269636050940713694, _gamma = 1} > , SMGen {_seed = 11331281847540010644, _gamma = 1} > , SMGen {_seed = 5619286150931196027 , _gamma = 1} > , SMGen {_seed = 10281499533683189089, _gamma = 1} > , SMGen {_seed = 17485363145453140727, _gamma = 7} > , SMGen {_seed = 17937441790592959953, _gamma = 7} > , SMGen {_seed = 15529190628949695919, _gamma = 7} > , SMGen {_seed = 6957216782316743206 , _gamma = 7} > , SMGen {_seed = 13022414457527836317, _gamma = 15} > , SMGen {_seed = 2127073997383595602 , _gamma = 15} > , SMGen {_seed = 7685868475082380828 , _gamma = 15} > , SMGen {_seed = 6396586367974718702 , _gamma = 15} > , SMGen {_seed = 13838712901582905874, _gamma = 31} > , SMGen {_seed = 15464227283579875280, _gamma = 31} > , SMGen {_seed = 17142043965827948305, _gamma = 31} > , SMGen {_seed = 10266498716653543189, _gamma = 31} ] >Prelude System.Random.SplitMix> >``` It compares just *one* implementation of SplitMix, [this](https://github.com/phadej/splitmix/blob/f6895c4137b14e4752b0575c178199201eb41810/src/System/Random/SplitMix.hs) one, at some particular point in time. (I can see that the author went and fixed it recently as shown from the diff-output [here](haskellari/splitmix@bd0692b) and [here](haskellari/splitmix@c6ba2f3), so this means that now *The Bug* section of http://www.pcg-random.org/posts/bugs-in-splitmix.html is outdated.) --- Here's the exact same test with *Hedgehog.Internal.Seed*: ``` λ :t from from :: GHC.Word.Word64 -> Seed λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 7046029254386353131 11400714819323198485 , Seed 17885559969949501885 11400714819323198485 , Seed 9864166656744503064 11400714819323198485 , Seed 9188376289577737681 11400714819323198485 , Seed 8743694738624347980 11400714819323198485 , Seed 17928928724259255865 11400714819323198485 , Seed 11302328716527294647 11400714819323198485 , Seed 17584158569056203712 11400714819323198485 , Seed 3484863033197266566 11400714819323198485 , Seed 3863623312507393319 11400714819323198485 , Seed 13742489918233434733 11400714819323198485 , Seed 9675543792836633510 11400714819323198485 , Seed 17288824720004427893 11400714819323198485 , Seed 8746728143070965849 11400714819323198485 , Seed 420250731748546163 11400714819323198485 , Seed 8521984098521027180 11400714819323198485 ] λ ``` This looks weird, as I'm getting the *exact same γ* through all the seeds. If I change Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..fe1a0be 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -98,11 +98,11 @@ random = -- | Create a 'Seed' using a 'Word64'. -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) λ ``` I can now reproduce the bug [*exactly* as shown in the article](http://www.pcg-random.org/posts/bugs-in-splitmix.html#the-bug): ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 1 , Seed 11409286845259996466 1 , Seed 1931727433621677744 1 , Seed 307741759840609752 1 , Seed 8606169619657412120 7 , Seed 13651108307767328632 7 , Seed 125750466559701114 7 , Seed 6781260234005250507 7 , Seed 15306535823716590088 15 , Seed 7344074043290227165 15 , Seed 9920554987610416076 15 , Seed 3341781972484278810 15 , Seed 12360157267739240775 31 , Seed 600595566262245170 31 , Seed 1471112649570176389 31 , Seed 8100917074368564322 31 ] λ ``` This is actually great so far, as it points out that we got Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) wrong and we can now 'fix' it according to the 'reference' source. (At this point, I can tell that the only reliable source is the one found in Oracle's JDK8 implementation.) --- And finally, If I go and change the glorious Hedgehog.Internal.Seed [`mixGamma`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L202) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214) (essentially, reverting hedgehogqa#124): ```diff λ :! git diff diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs index 61cacb2..5428927 100644 --- a/hedgehog/src/Hedgehog/Internal/Seed.hs +++ b/hedgehog/src/Hedgehog/Internal/Seed.hs @@ -100,7 +100,7 @@ random = -- from :: Word64 -> Seed from x = - Seed x goldenGamma + Seed (mix64 x) (mixGamma (x + goldenGamma)) -- | A predefined gamma value's needed for initializing the "root" instances of -- 'Seed'. That is, instances not produced by splitting an already existing @@ -192,7 +192,7 @@ mixGamma x = y = mix64variant13 x .|. 1 n = popCount $ y `xor` (y `shiftR` 1) in - if n >= 24 then + if n < 24 then y `xor` 0xaaaaaaaaaaaaaaaa else y λ ``` It 'fixes' the gamma value: ``` λ Prelude.map from [ 0x61c8864680b583eb , 0xf8364607e9c949bd , 0x88e48f4fcc823718 , 0x7f83ab8da2e71dd1 , 0x7957d809e827ff4c , 0xf8d059aee4c53639 , 0x9cd9f015db4e58b7 , 0xf4077b0dbebc73c0 , 0x305cb877109d0686 , 0x359e58eeafebd527 , 0xbeb721c511b0da6d , 0x86466fd0fcc363a6 , 0xefee3e7b93db3075 , 0x79629ee76aa83059 , 0x05d507d05e785673 , 0x76442b62dddf926c ] [ Seed 15210016002011668638 12297829382473034411 , Seed 11409286845259996466 12297829382473034411 , Seed 1931727433621677744 12297829382473034411 , Seed 307741759840609752 12297829382473034411 , Seed 8606169619657412120 12297829382473034413 , Seed 13651108307767328632 12297829382473034413 , Seed 125750466559701114 12297829382473034413 , Seed 6781260234005250507 12297829382473034413 , Seed 15306535823716590088 12297829382473034405 , Seed 7344074043290227165 12297829382473034405 , Seed 9920554987610416076 12297829382473034405 , Seed 3341781972484278810 12297829382473034405 , Seed 12360157267739240775 12297829382473034421 , Seed 600595566262245170 12297829382473034421 , Seed 1471112649570176389 12297829382473034421 , Seed 8100917074368564322 12297829382473034421 ] λ ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See mixGamma http://gee.cs.oswego.edu/dl/papers/oopsla14.pdf on page 13.
Also see jane streets implementation for comparison https://github.com/janestreet/core_kernel/blob/master/src/splittable_random.ml#L87.
@jystic On a side note, there is a dedicated place for people who write bit masks in decimal, I dearly hope you have someone else to blame.