Skip to content

Random: make randperm! and randcycle! accept AbstractArray #58596

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented May 31, 2025

Relax the dispatch from Array to AbstractArray, while requiring one-based indexing.

Copy link

Hello! I am a bot.

Thank you for your pull request!

I have assigned @DilumAluthge to this pull request.

@DilumAluthge can either choose to review this pull request themselves, or they can choose to find someone else to review this pull request.

@nsajko nsajko added randomness Random number generation and the Random stdlib stdlib Julia's standard library feature Indicates new feature / enhancement requests arrays [a, r, r, a, y, s] status: waiting for PR reviewer and removed status: waiting for PR reviewer labels May 31, 2025
@nsajko
Copy link
Contributor Author

nsajko commented May 31, 2025

I feel like even the <:Integer constraints are not necessary, however it seems like that could be left for another PR.

@DilumAluthge DilumAluthge requested a review from rfourquet May 31, 2025 23:41
@DilumAluthge
Copy link
Member

@rfourquet Can you review this?

@nsajko nsajko added needs news A NEWS entry is required for this change needs compat annotation Add !!! compat "Julia x.y" to the docstring labels May 31, 2025
@nsajko nsajko force-pushed the stdlib_Random_randperm_randcycle_Array_to_AbstractArray branch from 2190c3d to 1154fd9 Compare June 1, 2025 00:04
@nsajko nsajko removed needs news A NEWS entry is required for this change needs compat annotation Add !!! compat "Julia x.y" to the docstring labels Jun 1, 2025
@nsajko nsajko force-pushed the stdlib_Random_randperm_randcycle_Array_to_AbstractArray branch from 6bf6bb5 to 45a4855 Compare June 1, 2025 09:43
@nsajko nsajko added the needs tests Unit tests are required for this change label Jun 1, 2025
Relax the dispatch from `Array` to `AbstractArray`, while requiring
one-based indexing.
@nsajko nsajko force-pushed the stdlib_Random_randperm_randcycle_Array_to_AbstractArray branch from 45a4855 to c2cb419 Compare June 1, 2025 09:52
@nsajko nsajko removed the needs tests Unit tests are required for this change label Jun 1, 2025
@adienes
Copy link
Member

adienes commented Jun 1, 2025

I guess this would make the error message more confusing for BitArray, but Array{Bool} already had the same problem anyway

@DilumAluthge
Copy link
Member

I guess this would make the error message more confusing for BitArray, but Array{Bool} already had the same problem anyway

Perhaps we could add some error hints to help that situation?

@nsajko
Copy link
Contributor Author

nsajko commented Jun 1, 2025

Don't see anything special about Bool:

julia> using Random

julia> f(t::Type) = randperm!(Array{t}(undef, Int(typemax(t))::Int + 1))
f (generic function with 1 method)

julia> f(Bool)
ERROR: InexactError: Bool(2)
Stacktrace:
 [1] Bool
   @ ./bool.jl:191 [inlined]
 [2] convert
   @ ./number.jl:7 [inlined]
 [3] setindex!
   @ ./array.jl:985 [inlined]
 [4] randperm!(r::TaskLocalRNG, a::Vector{Bool})
   @ Random ~/tmp/jl/jl/julia-2190c3d1c4/share/julia/stdlib/v1.13/Random/src/misc.jl:330
 [5] randperm!(a::Vector{Bool})
   @ Random ~/tmp/jl/jl/julia-2190c3d1c4/share/julia/stdlib/v1.13/Random/src/misc.jl:336
 [6] f(t::Type)
   @ Main ./REPL[2]:1
 [7] top-level scope
   @ REPL[3]:1

julia> f(Int8)
ERROR: InexactError: trunc(Int8, 128)
Stacktrace:
  [1] throw_inexacterror(func::Symbol, to::Type, val::Int64)
    @ Core ./boot.jl:863
  [2] checked_trunc_sint
    @ ./boot.jl:877 [inlined]
  [3] toInt8
    @ ./boot.jl:892 [inlined]
  [4] Int8
    @ ./boot.jl:1002 [inlined]
  [5] convert
    @ ./number.jl:7 [inlined]
  [6] setindex!
    @ ./array.jl:985 [inlined]
  [7] randperm!(r::TaskLocalRNG, a::Vector{Int8})
    @ Random ~/tmp/jl/jl/julia-2190c3d1c4/share/julia/stdlib/v1.13/Random/src/misc.jl:330
  [8] randperm!(a::Vector{Int8})
    @ Random ~/tmp/jl/jl/julia-2190c3d1c4/share/julia/stdlib/v1.13/Random/src/misc.jl:336
  [9] f(t::Type)
    @ Main ./REPL[2]:1
 [10] top-level scope
    @ REPL[4]:1

julia> f(UInt8)
ERROR: InexactError: trunc(UInt8, 256)
Stacktrace:
  [1] throw_inexacterror(func::Symbol, to::Type, val::Int64)
    @ Core ./boot.jl:863
  [2] checked_trunc_uint
    @ ./boot.jl:885 [inlined]
  [3] toUInt8
    @ ./boot.jl:947 [inlined]
  [4] UInt8
    @ ./boot.jl:1007 [inlined]
  [5] convert
    @ ./number.jl:7 [inlined]
  [6] setindex!
    @ ./array.jl:985 [inlined]
  [7] randperm!(r::TaskLocalRNG, a::Vector{UInt8})
    @ Random ~/tmp/jl/jl/julia-2190c3d1c4/share/julia/stdlib/v1.13/Random/src/misc.jl:330
  [8] randperm!(a::Vector{UInt8})
    @ Random ~/tmp/jl/jl/julia-2190c3d1c4/share/julia/stdlib/v1.13/Random/src/misc.jl:336
  [9] f(t::Type)
    @ Main ./REPL[2]:1
 [10] top-level scope
    @ REPL[5]:1

@DilumAluthge
Copy link
Member

I'm assigning this to @rfourquet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] feature Indicates new feature / enhancement requests randomness Random number generation and the Random stdlib status: waiting for PR reviewer stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants