-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
This seems to introduce method ambiguities, so perhaps these should be resolved across the ecosystem first |
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. |
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
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. |
Instead of fighting packages over ambiguities, 3ee4ffc changes it to an explicit check with an error. (There's already a function called 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 |
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. |
@nanosoldier |
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")) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
This needs rebase before merging |
The package evaluation job you requested has completed - possible new issues were detected. |
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
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:After: