Skip to content
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

sample throws error when used on UInt64 #872

Closed
lampretl opened this issue Jun 30, 2023 · 7 comments · Fixed by #873
Closed

sample throws error when used on UInt64 #872

lampretl opened this issue Jun 30, 2023 · 7 comments · Fixed by #873

Comments

@lampretl
Copy link

lampretl commented Jun 30, 2023

In Julia 1.9.1 with StatsBase v0.34.0, the following code

T = UInt64;   
StatsBase.sample(T(1):T(10), T(1), replace=false, ordered=false)
StatsBase.sample(T(1):T(10), T(3), replace=false, ordered=false)

runs fine, but the code
StatsBase.sample(T(1):T(10), T(2), replace=false,ordered=false)
returns

ERROR: MethodError: no method matching samplepair(::Random._GLOBAL_RNG, ::UInt64)

Closest candidates are:
samplepair(::AbstractRNG, ::Int64)
@ StatsBase ~/.julia/packages/StatsBase/iMkPf/src/sampling.jl:108
samplepair(::AbstractRNG, ::AbstractArray)
@ StatsBase ~/.julia/packages/StatsBase/iMkPf/src/sampling.jl:123

Stacktrace:
[1] samplepair(rng::Random._GLOBAL_RNG, a::UnitRange{UInt64})
@ StatsBase ~/.julia/packages/StatsBase/iMkPf/src/sampling.jl:124
[2] sample!(rng::Random._GLOBAL_RNG, a::UnitRange{UInt64}, x::Vector{UInt64}; replace::Bool, ordered::Bool)
@ StatsBase ~/.julia/packages/StatsBase/iMkPf/src/sampling.jl:514
[3] sample!
@ ~/.julia/packages/StatsBase/iMkPf/src/sampling.jl:486 [inlined]
[4] #sample#209
@ ~/.julia/packages/StatsBase/iMkPf/src/sampling.jl:543 [inlined]
[5] sample
@ ~/.julia/packages/StatsBase/iMkPf/src/sampling.jl:541 [inlined]
[6] #sample#210
@ ~/.julia/packages/StatsBase/iMkPf/src/sampling.jl:545 [inlined]
[7] top-level scope
@ REPL[79]:1

However, the code below runs fine:

T = UInt32;   
StatsBase.sample(T(1):T(10), T(2), replace=false,ordered=false)
@ararslan
Copy link
Member

I have no idea why it works for UInt32... AFAICT it should be going through the same code paths as UInt64 which means it should get to the same samplepair call that's expecting an Int. Oddly, widths below 64 bits all seem to work fine:

julia> for T in [Int8, Int16, Int32, Int64, Int128, BigInt], f in [identity, unsigned]
           T == BigInt && f == unsigned && continue
           T = f(T)
           try
               sample(T(1):T(10), T(2); replace=false, ordered=false)
               @info "$T works"
           catch ex
               if ex isa MethodError && ex.f == samplepair
                   @info "$T does not work :("
               else
                   rethrow()
               end
           end
       end
[ Info: Int8 works
[ Info: UInt8 works
[ Info: Int16 works
[ Info: UInt16 works
[ Info: Int32 works
[ Info: UInt32 works
[ Info: Int64 works
[ Info: UInt64 does not work :(
[ Info: Int128 does not work :(
[ Info: UInt128 does not work :(
[ Info: BigInt does not work :(

At any rate, this fixes it:

--- a/src/sampling.jl
+++ b/src/sampling.jl
@@ -105,12 +105,12 @@ Draw a pair of distinct integers between 1 and `n` without replacement.
 Optionally specify a random number generator `rng` as the first argument
 (defaults to `Random.GLOBAL_RNG`).
 """
-function samplepair(rng::AbstractRNG, n::Int)
-    i1 = rand(rng, 1:n)
-    i2 = rand(rng, 1:n-1)
+function samplepair(rng::AbstractRNG, n::Integer)
+    i1 = rand(rng, Base.OneTo(n))
+    i2 = rand(rng, Base.OneTo(n - one(n)))
     return (i1, ifelse(i2 == i1, n, i2))
 end
-samplepair(n::Int) = samplepair(Random.GLOBAL_RNG, n)
+samplepair(n::Integer) = samplepair(Random.GLOBAL_RNG, n)

 """
     samplepair([rng], a)

@lampretl
Copy link
Author

lampretl commented Jul 1, 2023

Thank you for a quick reply. Looking forward to newer versions of Julia, where this is fixed.

Best regards

@ararslan
Copy link
Member

ararslan commented Jul 3, 2023

Do you mean you tried with a source build of Julia master or nightly binaries and this behavior does not occur?

@lampretl
Copy link
Author

lampretl commented Jul 3, 2023

Your code below

At any rate, this fixes it

Those are probably the changes you made to fix the problem.

I haven't tried anything new. I always used Julia master.
I thought the fix will come with Julia 1.9.2 or later. Till then, I will stick to UInt32.

Off-topic: It would be nice if StatsBase.jl were one day merged with Statistics.jl, as the former is not part of Base, but the latter is, so the naming is confusing.

@ararslan
Copy link
Member

ararslan commented Jul 3, 2023

It will be fixed in the next release of the StatsBase package; it is not tied to Julia itself. Once the release is available, updating the package environment(s) where you're using StatsBase should automatically give you the new version that contains the fix.

@lampretl
Copy link
Author

lampretl commented Jul 3, 2023

Thank you. Hopefully your edits won't have any performance degrading effects (Int=Int64 is a datatype of fixed size, while Integer is not). I really need StatsBase.sample and StatsBase.sample!to be fast.

@ararslan
Copy link
Member

ararslan commented Jul 3, 2023

There should be no adverse effect on performance with those changes. Integer is indeed an abstract type but that only affects what values are accepted by that method; Julia will still generate specialized code for different concrete subtypes, e.g. Int.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants