julia icon indicating copy to clipboard operation
julia copied to clipboard

Tighten signature of `repeat` to match what's documented

Open mcabbott opened this issue 1 year ago • 5 comments

Accidentally passing a float here (due to forgetting that ceil doesn't return an integer) gives an error that's more confusing than it has to be:

julia> repeat(1:3, 4.0)
ERROR: MethodError: no method matching similar(::UnitRange{Int64}, ::Float64)
The function `similar` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  similar(::JuliaInterpreter.Compiled, ::Any)
   @ JuliaInterpreter ~/.julia/packages/JuliaInterpreter/tYiNO/src/types.jl:7
  similar(::AbstractArray, ::Type{T}) where T
   @ Base abstractarray.jl:821
  similar(::AbstractArray, ::Type{T}, ::NTuple{N, Int64}) where {T, N}
   @ Base abstractarray.jl:833
  ...

Stacktrace:
 [1] repeat_outer(a::UnitRange{Int64}, ::Tuple{Float64})
   @ Base._RepeatInnerOuter ./abstractarraymath.jl:480
 [2] repeat_inner_outer(arr::UnitRange{Int64}, ::Nothing, outer::Tuple{Float64})
   @ Base._RepeatInnerOuter ./abstractarraymath.jl:460
 [3] repeat(arr::UnitRange{Int64}; inner::Nothing, outer::Tuple{Float64})
   @ Base._RepeatInnerOuter ./abstractarraymath.jl:402
 [4] repeat(A::UnitRange{Int64}; inner::Nothing, outer::Tuple{Float64})
   @ Base ./abstractarraymath.jl:394
 [5] repeat(A::UnitRange{Int64}, counts::Float64)
   @ Base ./abstractarraymath.jl:357
 [6] top-level scope
   @ REPL[48]:1

help?> repeat
search: repeat reset pad_repeat keepat! relpath replace read @deprecate realpath repr RecipeData

  repeat(A::AbstractArray, counts::Integer...)

  Construct an array by repeating array A a given number of times in each dimension, specified by
  counts.

  See also: fill, Iterators.repeated, Iterators.cycle.

After:

julia> repeat(1:3, 4.0)
ERROR: MethodError: no method matching repeat(::UnitRange{Int64}, ::Float64)
The function `repeat` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  repeat(::AbstractArray, ::Integer...)
   @ Base REPL[4]:1

mcabbott avatar Aug 10 '24 03:08 mcabbott

This seems to introduce method ambiguities, so perhaps these should be resolved across the ecosystem first

jishnub avatar Aug 12 '24 05:08 jishnub

Error is this:

  MethodError: repeat(::SparseArrays.SparseMatrixCSC{Float64, Int64}, ::Int64) is ambiguous.
  Candidates:
    repeat(A::SparseArrays.AbstractSparseMatrixCSC, m)
      @ SparseArrays /cache/build/tester-amdci4-14/julialang/julia-master/julia-d4b18e6530/share/julia/stdlib/v1.12/SparseArrays/src/sparsematrix.jl:3974
    repeat(A::AbstractArray, counts::Integer...)
      @ Base abstractarraymath.jl:435
  Possible fix, define
    repeat(::SparseArrays.AbstractSparseMatrixCSC, ::Integer)

Unfortunately I imagine there will be many such errors from packages, too.

mcabbott avatar Aug 12 '24 18:08 mcabbott

@nanosoldier runtests()

LilithHafner avatar Aug 18 '24 12:08 LilithHafner

The package evaluation job you requested has completed - possible new issues were detected. The full report is available.

nanosoldier avatar Aug 19 '24 22:08 nanosoldier

I see one real failure in Alicorn.jl and many packages that failed CI due to Aqua eagerly detecting a possible ambiguity but not actually hitting it.

LilithHafner avatar Aug 19 '24 23:08 LilithHafner

Instead of fighting packages over ambiguities, 3ee4ffc changes it to an explicit check with an error. (There's already a function called check performing other sanity checks, such as negative repeats.) The example above becomes:

julia> repeat(1:3, 4.0)
ERROR: ArgumentError: repeat requires integer counts, got outer = (4.0,)
Stacktrace:
 [1] check(arr::UnitRange{Int64}, inner::Nothing, outer::Tuple{Float64})
   @ Base._RepeatInnerOuter ./REPL[30]:18
 [2] repeat(arr::UnitRange{Int64}; inner::Nothing, outer::Tuple{Float64})
   @ Base._RepeatInnerOuter ./abstractarraymath.jl:399
 [3] repeat(A::UnitRange{Int64}, counts::Float64)
   @ Base ./abstractarraymath.jl:356
 [4] top-level scope
   @ REPL[32]:1

mcabbott avatar Nov 09 '24 02:11 mcabbott

I expect this to be clean

@nanosoldier runtests()

Going forward, folks can define specializations with the latter arguments typed as Integers which could be a path to turning this into a more appropriate (and simpler) method error.

LilithHafner avatar Nov 09 '24 02:11 LilithHafner

@nanosoldier runtests()

LilithHafner avatar Nov 09 '24 02:11 LilithHafner

Yes.

One advantage is that this also checks keywords inner/outer. Those can't easily have tight signatures as they accept almost anything:

julia> repeat([1 2 3]; inner=transpose(2:3))
2×9 Matrix{Int64}:
 1  1  1  2  2  2  3  3  3
 1  1  1  2  2  2  3  3  3

julia> repeat([1 2 3]; outer=(i^2 for i in 1:2))
1×12 Matrix{Int64}:
 1  2  3  1  2  3  1  2  3  1  2  3

mcabbott avatar Nov 09 '24 02:11 mcabbott

This needs rebase before merging

giordano avatar Nov 09 '24 11:11 giordano

The package evaluation job you requested has completed - possible new issues were detected. The full report is available.

nanosoldier avatar Nov 09 '24 15:11 nanosoldier

@nanosoldier runtests(["LinearRationalExpectations", "EarlyStopping", "PermutationGroups", "Groups", "SimpleWebsockets", "GraphViz", "GLPK", "RandomLinearAlgebraSolvers", "SliceSampling", "SteadyStateDiffEq", "ODEInterfaceDiffEq", "MCMCTempering", "MLJTestIntegration", "UltraDark", "MimiRFFSPs", "Population", "IterativeLearningControl"])

LilithHafner avatar Nov 10 '24 19:11 LilithHafner

The package evaluation job you requested has completed - possible new issues were detected. The full report is available.

nanosoldier avatar Nov 11 '24 10:11 nanosoldier