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

Amended constant value used by the pseudo-random number generator #636

Merged
merged 1 commit into from Jun 30, 2016

Conversation

Projects
None yet
2 participants
@mariosangiorgio
Contributor

mariosangiorgio commented Jun 2, 2016

As I mentioned in my comment in issue #635, I think the constant we're using needs to be amended. The Haskell implementation uses that value:

mkStdGen32 sMaybeNegative = StdGen (s1+1) (s2+1)
    [...]
    s2      = q `mod` 2147483398

stdNext (StdGen s1 s2) = (fromIntegral z', StdGen s1'' s2'')
        [...]
        s2'' = if s2' < 0 then s2' + 2147483399 else s2'

stdSplit std@(StdGen s1 s2)
            [...]
                        new_s2 | s2 == 1          = 2147483398
            [...]

The same value is mentioned in the original paper.

For 32-bit computers, we suggest l= 2, m1 = 2147483563, a1 = 40014, m2 = 2147483399 and a2 =
40692.

I am by no means expert on pseudo-random number generators, so I have no idea why we might have been using a different values. I believe that's a typo but please double check it before accepting this pull request.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jun 26, 2016

Member

Thanks! There is an alternate implementation of PRNG in the pipeline, so I think it makes sense to prefer that route.

Member

evancz commented Jun 26, 2016

Thanks! There is an alternate implementation of PRNG in the pipeline, so I think it makes sense to prefer that route.

@evancz evancz closed this Jun 26, 2016

@evancz evancz reopened this Jun 30, 2016

@evancz evancz merged commit df95a9e into elm:master Jun 30, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jun 30, 2016

Member

Actually, it does make sense to merge this in. Great find, thank you for fixing it in a PR!

Member

evancz commented Jun 30, 2016

Actually, it does make sense to merge this in. Great find, thank you for fixing it in a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment