unify shuffle and randperm

[ctarn]

unify the algorithms of shuffle and randperm without performance loss, so that:

• shuffle(rng, 1:n) == randperm(rng, n)
• shuffle(rng, eachindex(1:n)) == shuffle(rng, 1:n)

see also:

• shuffle works differently on Base.OneTo than others #50304
• Dispatch shuffle(::Base.OneTo) to randperm #45732

correctness:

ns = [1, 10, 100, 1000, 10000]

for n in ns
    @show shuffle!(MersenneTwister(1), Vector(1:n) .* 7) ./ 7 == randperm(MersenneTwister(1), n)
end

"""
output:
shuffle!(MersenneTwister(1), Vector(1:n) .* 7) ./ 7 == randperm(MersenneTwister(1), n) = true
shuffle!(MersenneTwister(1), Vector(1:n) .* 7) ./ 7 == randperm(MersenneTwister(1), n) = true
shuffle!(MersenneTwister(1), Vector(1:n) .* 7) ./ 7 == randperm(MersenneTwister(1), n) = true
shuffle!(MersenneTwister(1), Vector(1:n) .* 7) ./ 7 == randperm(MersenneTwister(1), n) = true
shuffle!(MersenneTwister(1), Vector(1:n) .* 7) ./ 7 == randperm(MersenneTwister(1), n) = true
"""

performance:

ns = [1, 10, 100, 1000, 10000]

t_new = [@belapsed(shuffle!(MersenneTwister(1), Vector(1:$n))) for n in ns]
t_old = [@belapsed(Random.shuffle!(MersenneTwister(1), Vector(1:$n))) for n in ns]
println("small item:\n#item\tnew\told")
display(hcat(ns, t_new, t_old))

"""
output:
small item:
#item   new     old
5×3 Matrix{Float64}:
     1.0  9.75e-6    9.748e-6
    10.0  9.938e-6   1.02e-5
   100.0  1.0523e-5  1.0548e-5
  1000.0  1.3765e-5  1.3549e-5
 10000.0  7.2876e-5  7.1795e-5
"""

vs = [[NTuple{1000, Float64}(rand(1000)) for _ in 1:n] for n in ns]
t_new = [@belapsed(shuffle!(MersenneTwister(1), $v)) for v in copy.(vs)]
t_old = [@belapsed(Random.shuffle!(MersenneTwister(1), $v)) for v in copy.(vs)]
println("large item:\n#item\tnew\told")
display(hcat(ns, t_new, t_old))

"""
output:
large item:
#item   new     old
5×3 Matrix{Float64}:
     1.0  9.811e-6     9.733e-6
    10.0  1.2482e-5    1.2365e-5
   100.0  4.4791e-5    4.3842e-5
  1000.0  0.000401874  0.000391641
 10000.0  0.00989083   0.0101406
"""

[stevengj]

It makes sense not to me not to have shuffle and randperm use gratuitously different algorithms. Would be good for someone who implemented these (e.g. @rfourquet way going way back to #14772) to take a look, however.

[ctarn]

The difference can be traced back to the initial implementation of 13 years ago by @ViralBShah:

julia/combinatorics.j

Lines 151 to 169 in 18b72d1

	# Knuth shuffle
	function shuffle(a::Vector)
	for i = length(a):-1:2
	j = randint(i)
	a[i], a[j] = a[j], a[i]
	end
	return a
	end
	function randperm(n::Int)
	a = Array(typeof(n), n)
	a[1] = 1
	for i = 2:n
	j = randint(i)
	a[i] = a[j]
	a[j] = i
	end
	return a
	end

@rfourquet made an improvement in #14772 while preserving the original style.

I don't see any reason why they are different, and since shuffle on Base.OneTo(n) is currently specialized as randperm(n), it is confusing that shuffle(collect(Base.OneTo(n))) != shuffle(Base.OneTo(n)) while collect(Base.OneTo(n)) == Base.OneTo(n).

julia/stdlib/Random/src/misc.jl

Line 267 in e4600c5

shuffle(r::AbstractRNG, a::Base.OneTo) = randperm(r, last(a))

julia> n = 4

julia> shuffle(MersenneTwister(1), collect(Base.OneTo(n)))
4-element Vector{Int64}:
 1
 2
 4
 3

julia> shuffle(MersenneTwister(1), Base.OneTo(n))
4-element Vector{Int64}:
 1
 3
 4
 2

julia> collect(Base.OneTo(n)) == Base.OneTo(n)
true

julia> shuffle(MersenneTwister(1), collect(Base.OneTo(n))) == shuffle(MersenneTwister(1), Base.OneTo(n))
false

[Seelengrab]

• shuffle(rng, 1:n) == randperm(rng, n)
• shuffle(rng, eachindex(1:n)) == shuffle(rng, 1:n)

Why is it advantageous to guarantee that these properties hold? It's all randomness either way - I don't think guaranteeing a particular result is desirable, as that will likely constrain any performance or other improvements to these algorithms in the future.

Not to mention that MersenneTwister hasn't been the default RNG for some time now, and showing that this property holds for MersenneTwister(1) and n == 4 says nothing about whether this holds in general.

[oscardssmith]

It's not a major advantage, but if it's easy to do it seems worthwhile

[stevengj]

Also, it makes the source code less readable/maintainable if shuffle and randperm use gratuitously different algorithms for essentially the same task.

[rfourquet]

LGTM

[Seelengrab]

If we want to guarantee these properties, this PR also should check that that they hold in a test.

[ctarn]

If we want to guarantee these properties, this PR also should check that that they hold in a test.

It can prevent some further improvements, although this is unlikely to happen. I am happy to add some tests, but don't insist on it.

These properties can also be documented, but don't have to be, I think.

[rfourquet]

Thanks @ctarn !