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

Tighten signature of repeat to match what's documented #55444

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented Aug 10, 2024

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 mcabbott added the arrays [a, r, r, a, y, s] label Aug 10, 2024
@jishnub
Copy link
Contributor

jishnub commented Aug 12, 2024

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

@mcabbott
Copy link
Contributor Author

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.

@LilithHafner
Copy link
Member

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

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

@LilithHafner
Copy link
Member

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.

@mcabbott
Copy link
Contributor Author

mcabbott commented Nov 9, 2024

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

@LilithHafner
Copy link
Member

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
Copy link
Member

@nanosoldier runtests()

@LilithHafner LilithHafner added needs pkgeval Tests for all registered packages should be run with this change merge me PR is reviewed. Merge when all tests are passing error messages Better, more actionable error messages labels Nov 9, 2024
@mcabbott
Copy link
Contributor Author

mcabbott commented Nov 9, 2024

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

@@ -518,6 +518,9 @@ function check(arr, inner, outer)
# TODO: Currently one based indexing is demanded for inner !== nothing,
# but not for outer !== nothing. Decide for something consistent.
Base.require_one_based_indexing(arr)
if !all(n -> n isa Integer, inner)
throw(ArgumentError("repeat requires integer counts, got inner = $inner"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this error message may be a lazy string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be. For now I left it just like all others in this check function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine this branch will DCE much of the time

@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Nov 9, 2024
@giordano
Copy link
Contributor

giordano commented Nov 9, 2024

This needs rebase before merging

@nanosoldier
Copy link
Collaborator

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

@LilithHafner
Copy link
Member

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

@nanosoldier
Copy link
Collaborator

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

@LilithHafner LilithHafner removed the needs pkgeval Tests for all registered packages should be run with this change label Nov 11, 2024
@LilithHafner LilithHafner merged commit ad24368 into JuliaLang:master Nov 11, 2024
8 of 9 checks passed
@mcabbott mcabbott deleted the repeat_int branch November 11, 2024 17:08
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] error messages Better, more actionable error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants