-
Notifications
You must be signed in to change notification settings - Fork 421
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
Refactor Random module #14435
Refactor Random module #14435
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.
I'm on board with renaming RandomStream to PCGRandomStream.
I'm unconvinced that we need to deprecate makeRandomStream.
@@ -203,14 +204,29 @@ module Random { | |||
seed: int(64) = SeedGenerator.oddCurrentTime, | |||
param parSafe: bool = true, | |||
param algorithm = defaultRNG) { | |||
compilerWarning("makeRandomStream is deprecated - " + | |||
"please use RandomStream.create instead"); |
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.
It's not obvious to me why we need to deprecate makeRandomStream. It's doing something slightly different, isn't it? The param algorithm
pattern exists also for a bunch of other functions here, so it seems to me like we should keep it.
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.
My main motivation was because it was one of the factory functions that are listed in #14291.
I understand your point that this is not a typical function, but at the same time I think it should follow the "convention" we are trying to set somehow, so maybe it should be just renamed to createRandomStream
and we should ditch the idea of having RandomStream.create
because it is not much different than new owned RandomStream
as you point out below?
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.
It seems to me, if we want to update for #14291, we should just rename the function to createRandomStream
.
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.
Only loosely following the discussion and PR(s), but it didn't seem like #14291 had reached a consensus on when to use type.create()
vs. createType()
from what I can tell.
so maybe it should be just renamed to createRandomStream and we should ditch the idea of having RandomStream.create because it is not much different than new owned RandomStream as you point out below?
If this is the case, maybe we should consider removing createRandomStream
and have users call new
. Does the factory function save much effort compared to new owned RandomStream
?
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.
It allows one to easily parameterize the RNG used (with the algorithm argument)
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.
OK, and just so I understand, does this represent the interfaces being considered?
RandomStream.create(algorithm=PCG);
// same as
PCGRandomStream.create();
// same as
new owned PCGRandomStream();
//
// vs.
//
createRandomStream(algorithm=PCG);
// same as
new owned PCGRandomStream();
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.
I think we decided not to make PCGRandomStream.create();
.
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.
@ben-albrecht We currently have the second version of those, but the alternative was slightly different than your 1st version. It is more like
RandomStream.create();
// same as (if defaultRNG==RNG.PCG, which is the default)
PCGRandomStream.create();
// same as
new owned PCGRandomStream();
My point being RandomStream
is only a type alias to the default type, and not a type itself.
:type parSafe: `bool` | ||
|
||
:returns: an owned PCGRandomStream | ||
*/ |
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.
If we keep this change, the documentation & method should also appear in RandomStreamInterface
:arg parSafe: The parallel safety setting. Defaults to `true`. | ||
:type parSafe: `bool` | ||
|
||
:returns: an owned RandomStream |
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.
I don't understand why we need type create
when a regular new
will do.
Isn't NPBRandomStream.create(int, seed)
redundant with new owned NPBRandomStream(int, seed)
?
I didn't want to rebase this branch and lose this history (just in case) to address the feedback. I am closing this PR. #14452 supersedes this. |
This PR refactors the Random module as documented in several notes.
Specifically:
RandomStream
class is renamed toPCGRandomStream
.RandomStream
is now a type alias for the default random stream.defaultRNG
is now aconfig param
instead of justparam
.makeRandomStream
is now deprecated.NPBRandomStream
andPCGRandomStream
now havecreate
type methods.private proc chpl_createStream
can be used to create a random stream similarto now-deprecated
makeRandomStream
As a result, several modules and tests are modified to use the new factory
function.
Test: