Skip to content
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

Turns out seedRandom doesn't work on unix platforms. This fixes that #2

Merged
2 commits merged into from
Jan 11, 2011
Merged

Turns out seedRandom doesn't work on unix platforms. This fixes that #2

2 commits merged into from
Jan 11, 2011

Conversation

DRMacIver
Copy link
Contributor

Totally trivial to fix. Added a call to seedRandom with a numeric argument in the tests file so that it would get caught if it happened again.

…dom where everything else in the library passes and expects a UInt. Unsurprisingly this didn't work
@jckarter
Copy link
Owner

test/random.clay doesn't build with your patch (at least on OS X) because there are some type mismatches remaining. Could you fix them?

Now that I look at it, the implementation of random() in the random module is totally bogus and doesn't return uniformly distributed values. uniform_random(a)/uniform_random(b) gives a distribution proportional to the denominator, since 1/2=2/4=3/6=... . random() should probably die and randomInt should be rewritten to just forward a random integer from the system random.

@DRMacIver
Copy link
Contributor Author

Interesting. I had exactly the opposite: test/random.clay didn't build on my platform because there were type mismatches! I don't have access to an OSX system, but I'll have a look at the code and see if I can get to the bottom of it.

@DRMacIver
Copy link
Contributor Author

No, my mistake. I was just an idiot and made changes after I tested it. This should work now.

@DRMacIver
Copy link
Contributor Author

And yes, I agree that the implementation looks quite suspicious. I was just picking off the low hanging fruit of it not actually working at all. :-)

@jckarter
Copy link
Owner

Works on my end with your fix too. I pulled the patch.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants