fix infinite loop in rand(::Range(BigInt))

[rfourquet]

The call rand(big(1):big(2)) was causing a stack overflow. It's not the best possible fix, as this works now only for ranges whose length fit in an Int. The simplest other solutions I see is to use rand(UnitRange{Uint128}) to implement rand(r::Range{BigInt}) (for big ranges of length up to typemax(Uint128)), or to constrain the T in rand{T}(r::Range{T}) (random.jl line 210) to prevent (hopefully temporarily) a call to rand with a big range altogether.

[rfourquet]

For a more satisfactory solution, see also @andrioni's PR, which was mergeg but reverted.

[JeffBezanson]

I'd prefer an error message or a better solution. How about using a sequence of random Bools to select an element of the range by binary search? Getting the booleans by manually taking bits out of a rand(Uint64) could probably be pretty fast.

[rfourquet]

Oh, I was lazy but since you asked... I'm not sure about your binary search idea: is it equivalent to the following? to generate a random BigInt in 1:n, generate x in 1:nextpow2(n,2) (easy) and then multiply x by n/nextpow2(n,2) (gives a BigFloat) and then "round" back to a BigInt. I am not familiar with these questions, but I don't think the "round" can be easily defined to obtain a uniform distribution (ceil fails).
So in this updated PR, I generate x in 1:nextpow2(n,2) until it is in 1:n.
I introduce RandIntGenBigInt which would correspond to a class template specialization of RandIntGen in C++, not sure if it is the best approach (there is a RandIntGen constructor which returns an instance of RandIntGenBigInt, which is not pretty).

[rfourquet]

The linux Travis build doesn't pass because of missing __gmpz_limbs_write, which I think was added in gmp 6.

[rfourquet]

The tests related to BigInt from
random.jl could be re-added... Should this be asked to the initial author, or anyone can do?

[ivarne]

I think there is a deeper issue in the design of rand(::Range) that calls rand(::UnitRange{T}) which is defined only for a specific union of T. Collections might decide any Number a valid return value from length(r).

It seems like what we need is a rand_int{T<:Integer}(n::T) function that returns a random Integer of type T between 1 and n inclusive (so that it can be used for indexing). That way all of, rand(v::AbstractVector) can be implemented as v[rand_int(length(a))], as I partally did in #8273.

@rfourquet This PR repurposes the RandIntGen() as a constructor for type RandIntGen to a generic constructor for a RandomIntegerGenerator. I think we should consider a rename.

I think anyone can re-add previously removed tests, but it's great if you note in the commit message where you got it from.

[rfourquet]

I totally agree: the stack overflow problem remains for all types not in the Union. I also really like your suggestion of having rand_int and rand(::AbstractVector) (cf. also #8257 (comment)). Currently I find RandIntGen a bit weird as its a field gives it an overlapping functionality with UnitRange.

WRT the constructor name problem, I don't have good ideas. WDYT of replacing outer constructors taking a UnitRange by a lower-case version: randintgen(r) returns a RandIntGen or a RandIntGenBigInt. In this case, your examples above can be written as v[rand(randintgen(length(a)))] (or v[randintgen(length(a))()] when instances can be "called").

[rfourquet]

@ivarne So I built upon your branch at https://github.com/rfourquet/julia/tree/rand_range. There, randintgen is the interface to build a RandIntGen object (or RandIntGenBigInt from my branch big-rand), and rand is extended to work with AbstractArray. I really don't know if this is an OK change (I don't know the Julia codebase), but at least the random tests pass, and it's nice to be able to write e.g. rand([2, 3, 5, 7]) :)

[rfourquet]

ps: I removed the comment or zero for full range of the .k field because it doesn't make much sense to me: there is already rand!(A::Array) for full range; but maybe it is used elsewhere? So I didn't remove the use of rem_knuth ...

[rfourquet]

I updated the current PR with re-added tests from commit bf8c452.

[rfourquet]

@ivarne do you think that I should do a PR of my branch rand_range above (which I'm rebasing now)? or do you want to do a PR yourself as this is your ideas, or does it have to wait for #6003 to be resolved? (in which case I will rebase this PR (rand(::Range{BigInt})) onto current master without waiting, including only the randintgen part).

[ivarne]

I think I've widened this discussion a little too much, so I'm starting to loose the overview. I hope I'm not to confusing.

Breaking changes to public APIs should not be done in multiple iterations, and should wait for a potential API redesign #6003. The question is what is public in this case. I tried to checkout all packages registered in METADATA.jl, and none of them contain the string RandIntGen, so it seems safe to change that without deprecations.

If I'm to decide, I would first get a decision on your organisation changes in your rand_range branch, before adding support for Range{BigInt}. I really like that you remove the a::T # first element of the range field, and make it implicitly 1. I don't know how others feels though, and there might be a reason for the original design.

[rfourquet]

@ivarne Thanks for checking METADATA.jl, I didn't think of that. So I will do a PR, as I also think that this change is not really breaking and sufficiently small to not complicate #6003.

[rfourquet]

Superseded by #9122.