randjump: deprecate the array version?

[rfourquet]

Original title: randjump: pass a seed instead of an RNG. Which was a bad idea. See the post below for updated proposition.

Another pretty late change, but this could be seen as a bug fix. This updates the API from randjump(rng::MersenneTwister, steps, len) to randjump(seed, steps, len). The two reasons I see for doing so are:

1. randjump called on freshly initialized RNGs works exactly, but is a bit off for already used RNGs. Eg:

julia> m = MersenneTwister(0);

julia> rand(m);

julia> x = randjump(m, 2^30, 2)[2];

julia> rand(m);

julia> y = randjump(m, 2^30, 2)[2];

julia> rand(x) == rand(y)
true

i.e. x == y (when #27744 gets merged) when they should be shifted away by 1 position;

2. I don't find a compelling reason to put an already existing RNG as the first element of the array produced by randjump. I could be very wrong on that, though. On github, I could find only very few uses of randjump, among which all but 2 (here and there, cc. @bkamins and @vvjn) were artificially creating a temporary RNG just to pass it to randjump (i.e. mt = MersenneTwister(seed); mts = randjump(mt, steps, len); ... # mt not used anymore).

Because of the fact that MersenneTwister has caches for Float64 and Integer values, it's not clear at all that 1) can be solved in a satisfying way while keeping the current API. Admitedly the intended use case for randjump should not be so concerned by this bug. While working on #25129 few months ago, I had to implement an exact version of randjump, but with aditional parameters to resolve ambiguities (so we may revisit this API when that PR gets finished, likely after 1.0).

cc. @wildart

[vvjn]

It seems to me like the problem is that randjump(rng::MersenneTwister, steps, len) is a bit off, i.e.,

julia> m = MersenneTwister(0);

julia> rand(m);

julia> x = randjump(m, 20);

julia> rand(m);

julia> y = randjump(m, 20);

julia> x==y
true

and therefore randjump(rng::MersenneTwister, steps, len) should be fixed instead of removing it. I'm not an expert on RNGs but maybe fix it by the following (except do the copy inside randjump)?

julia> m = MersenneTwister(0);

julia> rand(m);

julia> x = randjump(copy(m), 20);

julia> rand(m);

julia> y = randjump(copy(m), 20);

julia> y==x
false

My reasoning for keeping this API is that will be possible to use a different RNG from MersenneTwister. The current randjump API would still work if a different RNG is used. With this new API randjump would be stuck with MersenneTwister. The reason I used randjump while passing it an RNG is so that I can replicate everything when I run it a second time. I could use a seed for it but then with the new API I wouldn't be able to use anything other than MersenneTwister.

[rfourquet]

Note that in your example code, copy doesn't solve the problem. The produced RNG arrays compare to false, but actually the RNGs in second position (x[2] and y[2] in your example) will still produce the same streams.

But you are of course right to point out the generality of the current API. That could be handled though, e.g. with randjump(MersenneTwister, seed, steps, len).

Alternatively, we could just document that the jump is not exact in some cases, and that this may be updated/fixed later in a breaking way.
In this case, I would still be annoyed with the passed RNG aliasing the first element of the array: what do you think of

1. making a copy of the passed RNG (randjump(rng, ...)[1] is a copy of rng) ?
2. we don't put the passed RNG into the array, i.e. the result would be equal to what randjump(rng, ...)[2:end] gives currently ? The user can still get the current behavior by using pushfirst!.

[vvjn]

Yes you're right. The copy doesn't actually solve the problem. I would like to keep the current API and document the current problem, since the current API is generalizable. I do think that the first element that's returned being aliased to the input is unexpected and should be changed. I am leaning towards your second solution because of the following:

julia> m = MersenneTwister(0);

julia> rand(m);

julia> x = randjump(copy(m), 5);

julia> rand(m);

julia> y = randjump(copy(m), 5);

julia> x .== y
5-element BitArray{1}:
 false
  true
  true
  true
  true

That is, elements x[2:end] are newly seeded RNGs (sorry, don't know the terminology) while x[1] is an RNG which has already had many rand calls that has modified it. With your second solution the returned RNGs will be consistent in some sense.

[ViralBShah]

Maybe an unexported Random.jump is a better name while we are at it? Another option is that jump could be a keyword argument to the Random.seed/seedrand/randseed function.

[rfourquet]

Maybe an unexported Random.jump is a better name while we are at it?

Makes sense, as this function is quite specific and will probably not be used by the majority of the users of Random.

jump could be a keyword argument to the Random.seed/seedrand/randseed function.

I find that would be quite nice actually. The thing is that one would then need to first create an RNG before calling seed on it, unless with have two functions, seedrand! and seedrand (which is not mutating, but create a new RNG).

Also, this does remove the current functionality of creating an array, which I really like actually. From randjump(rng, steps) which creates a new RNG (currently in the Future module), it's simple to get the current behavior of randjump via accumulate: accumulate(randjump, MersenneTwister(0), fill(2^30, 3)), so it would not be necessary to provide the array version directly.

[vvjn]

I like randjump(rng::MersenneTwister, steps) :: MersenneTwister too since I would like some mechanism to generate a new RNG from a current RNG. One reason is to be able to replicate the output of a function that takes as input one RNG and then uses a separate RNG for each thread. The RNG for each thread need to be dependent on the single input RNG in order for the function to be replicable.

[rfourquet]

I think my prefered solution is then to delete the randjump method which produces an array, and deprecate it to the accumulate call which I showed in my previous post; it uses Future.randjump, which in 1.0 can I think be moved from Future to the Random module. Like that we have the building block, and we have more time to decide how to design a more convenient method to create arrays, according to the use-cases.

[ViralBShah]

Just so you know, things are getting locked down for the release. If this isn't fully settled, it will have to be a post 1.0 thing.

[rfourquet]

Just so you know, things are getting locked down for the release. If this isn't fully settled, it will have to be a post 1.0 thing.

OK, on my side this is settled, I would propose the plan from my last post, and it's a no-brainer to implement. @vvjn seems to be on the same page, but more people would need to voice their opinion to take a decision. Maybe I should just label this for triage?

[vvjn]

My preference is to remove the aliasing in either of the two proposed ways above, while keeping the array generation. The no array generation is okay with me (it's obviously not trivial to generate an array anymore though) but it's my 2nd preference.

Furthermore, the removing of array generation to the new API changes the current API in a breaking way. I don't think breaking API changes should be done anymore.

The only real problem I see is the aliasing of the first element and lack of documentation.

[rfourquet]

My preference is to remove the aliasing in either of the two proposed ways above, while keeping the array generation

Note that this would also be a breaking change.

[rfourquet]

Small recap for triage:

• in 0.6, we had two randjump methods (now deprecated) generating an array, one of them with a default number of steps: randjump(r::MersenneTwister, len::Integer) and randjump(r::MersenneTwister, len::Integer, jumppoly::AbstractString)
• in 0.7 currently, we have: randjump(r::MersenneTwister, steps::Integer, len::Integer) which behaves similarly to the methods above, but receives the number of steps to skip as an integer instead of as a string. There is also Future.randjump(r::MersenneTwister, steps::Integer) which returns a single RNG.
• @vvjn and I believe that it's not a good idea to have the array version returning the input RNG as the first element of the array; 3 possible solutions have been mentioned here:

1. making a copy of the passed RNG (randjump(rng, ...)[1] is a copy of rng)
2. we don't put the passed RNG into the array, i.e. the result would be equal to what randjump(rng, steps, len+1)[2:end] gives currently. The user can still get the current behavior by using pushfirst!
3. deprecate alltogether the array version in favor of a combination of the scalar version from Future and accumulate (accumulate(Future.randjump, MersenneTwister(0), fill(2^30, 3))), and wait that more use-cases give more insight as to what would be most useful in an array version

I vote for 3). Solution 1) doesn't seem ideal to me, as it's like we try to guess what the user needs, and the first element would actually not be "jumped" (same problem with the status quo).

Also, Viral suggested to rename randjump to Random.jump, which I quite like.

[StefanKarpinski]

@rfourquet: I think you're really the person to make this call. Random is not in Base so making breaking changes in a 1.x version of Julia is possible.

[JeffBezanson]

I also agree with option (3) above.

[rfourquet]

I think you're really the person to make this call

I know well the codebase, but I'm not a serious user of the vector version of randjump... anyway, I pushed an updated version implementing option 3, and I feel a great releaf about it.
Note that the already deprecated 0.6 version was not working (moving the deprecations to the Random module broke namespace resolution), and this has not been reported, which may indicate that close to no-one using randjump has already started to update her code to 0.7.
I will merge when green, if no objection.