Proposal: Random module interface changes

[e-kayrakli]

This issue is to propose small interface changes in the Random module.

Background

Under #14291, which was originally about creating a convention for standard
factory functions, we started discussing the Random module design and how its
interface can be adjusted to an agreed upon convention for factory functions.

Here are some relevant details about the things we have in the Random module as
today:

• Two classes that represent RNG algorithms:
  • PCGRandomStream
  • NPBRandomStream
• A type alias RandomStream that aliases the default RNG (PCG, see below)
• An enum RNG { PCG = 1, NPB = 2 }
• A param defaultRNG = RNG.PCG
• Besides methods on these RNG types, there are module-level convenience
  functions. These allow the user to create a random stream, or use some
  Random functionality without jumping through some hoops:
  • createRandomStream(...., algorithm=defaultRNG)
  • fillRandom(...., algorithm=defaultRNG)
  • permutation(...., algorithm=defaultRNG)
  • shuffle(...., algorithm=defaultRNG)

In #14291, the most support having the convention of type.create()
for factory functions, which createRandomStream does not adhere to. (There is
also an open PR to change all the remaining factory functions in the standard
library to use that convention: #14449). createRandomStream's merit is that it
parametrizes the algorithm. One way of making it comply with the
convention is to make it RandomStream.create, but it would not be very useful
in the existing interface because RandomStream is just an alias and this would
have an identical behavior to new [PCG]RandomStream().

Proposal

Based on my interpretation on the prior discussion under #14291, I propose we do
(all) the following:

1. Remove createRandomStream from the random module.

2. Make defaultRNG a config param (I thought I did that before, but I didn't).

3. Add module-level getRandomStreamType(param algorithm) to get the type
   associated with a given RNG algorithm.

   This is arguably an uglier way of parametrizing the algorithm. But I am not
   sure how common that is in the real world applications. I cannot think of any
   use case myself. So I'd be fine with that.

4. Make other module-level helpers type methods on RNG types.
   3a. Change fillRandom name to fill, so that we have
   [PCG|NPB]RandomStream.fill

   I am not sure how this would play out when/if module-level random streams are
   added (Simplify the construction of simple random numbers #7225), these may be confusing to the user:

   RandomStream.fill(...); // type method that creates a stream internally
   myRandomStream.fill(...); // regular method that uses myRandomStream
   Random.fill(...); // function that uses module-level stream

The above items are more-or-less mutually exclusive, so maybe a subset of them
is also a good approach.

Code Samples

Creating a random stream

// before
var rs = createRandomStream(...);
var rs = createRandomStream(..., algorithm=RNG.PCG);
var rs = createRandomStream(..., algorithm=RNG.NPB);

//after
var rs = new RandomStream(...);
var rs = new getRandomStreamType(algorithm=RNG.PCG)(...);
var rs = new getRandomStreamType(algorithm=RNG.NPB)(...);

Utility functions

// before
fillRandom(myArr, ...);
fillRandom(myArr, ..., algorithm=RNG.PCG);
fillRandom(myArr, ..., algorithm=RNG.NPB);

// after
RandomStream.fill(myArr, ...);
getRandomStreamType(algorithm=RNG.PCG).fill(myArr, ...);
getRandomStreamType(algorithm=RNG.PCG).fill(myArr, ...);

[vasslitvinov]

getRandomStreamType(algorithm=RNG.PCG).doSomething(...) seems excessively verbose. I propose instead:

RandomStream.doSomething(..., algorithm=RNG.PCG)

[e-kayrakli]

@vasslitvinov

Note that currently RandomStream is a type alias to the type that represent the default algorithm (PCG). And I am not sure what is a neat way of having what you propose.

[BryantLam]

Besides methods on these RNG types, there are module-level convenience
functions. These allow the user to create a random stream, or use some
Random functionality without jumping through some hoops:

• createRandomStream(...., algorithm=defaultRNG)
• fillRandom(...., algorithm=defaultRNG)
• permutation(...., algorithm=defaultRNG)
• shuffle(...., algorithm=defaultRNG)

// before
fillRandom(myArr, ...);
fillRandom(myArr, ..., algorithm=RNG.PCG);
fillRandom(myArr, ..., algorithm=RNG.NPB);

Scanning through the implementations of these convenience functions, I don't think there's a need to change them. These are fine because each function only creates a function-local random stream; these functions truly are a convenience for what the user could have done by hand in only one or two extra lines of code.

That said, if the goal is to remove global functions entirely, hey I'm okay with the changes! getRandomStreamType is certainly more verbose, but it's more apparent what is actually going on.

I don't really care enough about what this module should look like, so feel free to ignore my suggestions.

[mppf]

At present I don't think we should change anything about RandomStream. I really don't like the direction this is going. I think the whole discussion is confused about the purpose of a factory function.

If we wanted createRandomStream to be a type method, we could make RandomStream be an interface (if you imagine with me for a moment that interfaces exist at all). Then it would make sense for PCGRandomStream and NPBRandomStream to implement RandomStream and it would make sense for RandomStream.create() to be parametrized by which algorithm to use. This is arguably closer to the originally envisioned design for RandomStream. However we don't have interfaces yet.

As an alternative to an interface, we could consider making RandomStream be a parent class. We could do that now. Long ago, when I introduced PCGRandomStream, I didn't want to do that, for one reason or another. At the very least, if people actually used RandomStream.create(), it would return the parent class type, and make all the functions be dynamically dispatched. Which will be bad for performance and will also prevent people from using functions available in some RNGs but not in others.

Note also that if we were to make RandomStream be an interface or a parent class, then code like new RandomStream would no longer work. That means that by removing createRandomStream entirely we would only be making transition towards interfaces harder.

In particular the Random module has an idea of a default random stream. That default RNG should be easy to use. If you can't get it with new RandomStream, we should have an equally easy thing, like createRandomStream. new getRandomStreamType(algorithm=RNG.NPB)(...) is horrible. Requiring people to write new PCGRandomStream is also horrible.

[e-kayrakli]

After some offline discussions, we currently plan to hold off on this until we converged on a wider design discussions for standard library functions.

I will close until this becomes a bigger priority unless there is any objections.

[ben-albrecht]

I like the "RandomStream as a pseudo-interface proposal", and was planning on suggesting it myself.

I will close until this becomes a bigger priority unless there is any objections.

I don't have a strong opinion, but would be fine leaving it open as well.

[BryantLam]

I'll remark that createRandomStream(...) is a cleaner factory function than RandomStream.create(...) if RandomStream is some kind of interface because interfaces don't usually have a factory method that returns an implementer of that interface.

[BryantLam]

Eh. I take that back. You can make it work without it looking terrible. Just don't define the factory method inside the interface because it's not strictly part of the interface.

[e-kayrakli]

I am not sure how much I myself will be supportive of this proposal after some potential larger-scale style adjustments in the standard modules. I am closing this for now.