Add PCG Random Number Generator #2912
Merged
+1,954
−401
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
* factors seed generation into RandomSupport * takes actual implementation RNG out of Random * adds NBPRandom and PCGRandom that implement RandomStream
This is an intermediate state - we hope to move to PCGRandomStream as the default at some point. This change lets us get started without changing users of the RNGs.
|
@daviditen can you have an initial look at what I have here? |
David suggested it would be nicer to have one file with sub-modules, so here I rearrange the code to do that.
| /* | ||
| Generate a seed based on the current time in microseconds as | ||
| reported by :proc:`Time.getCurrentTime`, ensuring that it | ||
| meets the PRNG's requirements. |
There was a problem hiding this comment.
This comment is out of date, it leaves it to the caller to ensure the seed meets requirements now.
There was a problem hiding this comment.
Changed it to say " Each RNG should adjust a given seed to meet any requirements."
|
Testing has completed with 6 errors:
These look like minor issues to me but I will work through them. |
|
I lost the comment about shuffle/permute being on an existing RandomStream. That is OK with me. Since you're touching all the lines in Random.chpl anyway, could you reindent it please? Once you get the testing errors fixed I think this is looking good. |
RandomPrivate_iterate -> NPBRandomPrivate_iterate
|
|
Passed full testing. @daviditen, care to give the PR a final look over? |
|
Minor: The method "PCGRandomStream.getNext(min: eltType, max: eltType)" ignores its arguments and does the same thing as the 0-argument version of getNext. Otherwise, I think this is looking very good. |
Previous verison didn't produce a number in a range. Thanks to David for noticing the problem. Also added a test to check the behavior.
mppf
added a commit
that referenced
this pull request
Dec 16, 2015
Add PCG Random Number Generator * factors seed generation into RandomSupport * takes actual implementation RNG out of Random * sketch interface for RandomStream to be implemented by different generators * adds NPBRandom and PCGRandom that implement RandomStream * adds ldexp function to Math since it is needed to generate a random real from a random int The default RNG is still the NPB one. In the future I hope we will change to the PCG generator since it has better integer support. In addition, we need to be able to name different RNGs intelligently - right now RandomStream is the name of the NPB generator (to avoid making too many changes at once). Reviewed by @daviditen - thanks! Passed full local testing. TODO: I would like for RandomStream to be either a base class or a synonym for the default RNG. Unfortunately, neither of those work right now: * base class -> "Illegal super class" errors, since base class is generic * type RandomStream = PCGRandomStream; causes errors with a type constructor (seems to be missing generic-ness of type).
Closed
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.
The default RNG is still the NPB one. In the future I hope we will change
to the PCG generator since it has better integer support. In addition, we need
to be able to name different RNGs intelligently - right now RandomStream
is the name of the NPB generator (to avoid making too many changes
at once).
Reviewed by @daviditen - thanks!
Passed full local testing.
TODO:
I would like for RandomStream to be either a base class or a synonym for the default RNG.
Unfortunately, neither of those work right now:
causes errors with a type constructor (seems to be missing generic-ness of type).