add `shuffle(::NTuple)` to `Random` #56906

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.

Sign up for GitHub

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

Jump to bottom

Open

nsajko wants to merge 25 commits into JuliaLang:master from nsajko:shuffle_ntuple

Contributor

nsajko commented Dec 26, 2024

Fixes #56728

nsajko added randomness stdlib feature labels

nsajko commented

View reviewed changes

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

nsajko force-pushed the shuffle_ntuple branch from 7c9cbf9 to b2eb86a Compare

December 29, 2024 14:15

rfourquet added needs docs needs news labels

nsajko force-pushed the shuffle_ntuple branch from 213f598 to 7bb811d Compare

May 9, 2025 10:29

nsajko commented

View reviewed changes

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

nsajko commented

View reviewed changes

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

nsajko force-pushed the shuffle_ntuple branch from 70fa8ac to 4577162 Compare

May 9, 2025 10:39

nsajko added status: waiting for PR reviewer DO NOT MERGE and removed needs news needs docs status: waiting for PR reviewer labels

Contributor Author

nsajko commented May 9, 2025 •

edited

Loading

Unlike shuffle!, randperm! is currently restricted to Array, not admitting Memory, so this PR now depends on randperm! supporting Memory. Clearly it would be good to support more types than just Array with randperm! anyway. Will probably make another PR for that soon.

EDIT: decided to just forgo randperm! for now

nsajko commented

View reviewed changes

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

nsajko removed the DO NOT MERGE label

rfourquet reviewed

View reviewed changes

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

nsajko commented

View reviewed changes

stdlib/Random/src/misc.jl Show resolved Hide resolved

nsajko commented

View reviewed changes

stdlib/Random/test/runtests.jl Outdated Show resolved Hide resolved

nsajko added the status: waiting for PR reviewer label

rfourquet reviewed

View reviewed changes

stdlib/Random/test/runtests.jl Outdated Show resolved Hide resolved

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

stdlib/Random/src/misc.jl Outdated Show resolved Hide resolved

rfourquet added the needs compat annotation label

Member

rfourquet commented May 12, 2025

I'm surprised to see that your implementation allocates on the master branch, but not on v1.12, do you have an idea why that is the case? Not necessariy a problem though, it seems to be roughly the same speed.

nsajko force-pushed the shuffle_ntuple branch from f6839f7 to dcd43a7 Compare

May 12, 2025 10:10

nsajko added status: waiting for PR author and removed needs compat annotation status: waiting for PR reviewer labels

nsajko and others added 23 commits

June 10, 2025 17:58


          add shuffle(::NTuple) to Random

7936a9f

Fixes JuliaLang#56728


          rely on shuffle! and on escape analysis instead of on recursion

f3fb9f9


          bugfix: use provided pseudorandom generator

dc162c3


          simplify using randperm! as per rfourqet

2eee6e6


          delete unnecessary branch

8a8efff


          reindent

fcec466


          add news

9e9c1a5


          update doc string, and apply it to the function instead of to a method

ed3e09a


          use narrower type when possible as per rfourqet

5b50ae3


          de-hardcode typemax(UInt8)

90f1d2d


          safer

6e80b0d


          use shuffle! instead of randperm! again

18ededb


          relax one-arg method

c6c3997

Co-authored-by: Rafael Fourquet <fourquet.rafael@gmail.com>


          simpler dispatch constraint

4ab6f86

Co-authored-by: Rafael Fourquet <fourquet.rafael@gmail.com>


          delete now unnecessary method

d7191ef

Co-authored-by: Rafael Fourquet <fourquet.rafael@gmail.com>


          delete extraneous newline

8f3800f


          omit no alloc test set

ba162dc


          fix dispatch constraint: restrict to NTuple

912ec6c

Co-authored-by: Rafael Fourquet <fourquet.rafael@gmail.com>


          add compat annotation to doc string

35feb2d


          adjust "not identity at least once" test

bed23af


          expand comment

9944d1c


          simplify syntax, less let, etc

6ebd824


          use randperm! again, as it supports Memory now

d1ea278

The PR JuliaLang#58596 was merged.

nsajko force-pushed the shuffle_ntuple branch from 1e7a253 to d1ea278 Compare

June 10, 2025 16:08

Contributor Author

nsajko commented Jun 10, 2025

The PR #58596:

made randperm! support Memory, so it's now used instead of shuffle!
caused a merge conflict in NEWS.md, now fixed

nsajko and others added 2 commits

June 10, 2025 18:14


          restrict the one-arg method from Tuple to NTuple

f9dd73d


          Merge branch 'master' into shuffle_ntuple

157ad7a

nsajko mentioned this pull request

flaky TwicePrecision test #58726

Closed

Contributor Author

nsajko commented Jun 15, 2025

bump?

adienes reviewed

View reviewed changes

stdlib/Random/src/misc.jl

+                  #   WIP PR: https://github.com/JuliaLang/julia/pull/56167
+                  @inline let
+                      # use a narrow integer type to save stack space and prevent heap allocation
+                      Ind = if N ≤ typemax(UInt8)

Member

adienes Jun 16, 2025

for which sizes of tuples does this Ind help? is there a cutoff?

Contributor Author

nsajko Jun 16, 2025 •

edited

Loading

Yeah, there's a cutoff. The transformation of Memory heap allocations to stack allocations currently only works for Memory with small statically-known size. See PR #56847.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature randomness status: waiting for PR reviewer stdlib