JuliaArrays / OffsetArrays.jl

Fortran-like arrays with arbitrary, zero or negative starting indices.
Other
195 stars 40 forks source link

Make `AbstractArray` reshape method less specific #333

Closed jishnub closed 1 year ago

jishnub commented 1 year ago

Since this method is meant to be a fallback, this should be less specific than other methods in both the array type and the axis type. This fixes an ambiguity error while using OffsetArrays and FillArrays together:

julia> reshape(Fill(2,6), big(2), :)
ERROR: MethodError: reshape(::Fill{Int64, 1, Tuple{Base.OneTo{Int64}}}, ::Tuple{BigInt, Colon}) is ambiguous.

Candidates:
  reshape(A::AbstractArray, inds::Tuple{Union{Colon, Integer, AbstractUnitRange}, Vararg{Union{Colon, Integer, AbstractUnitRange}}})
    @ OffsetArrays ~/Dropbox/JuliaPackages/OffsetArrays.jl/src/OffsetArrays.jl:352
  reshape(parent::AbstractFill, dims::Tuple{Vararg{Union{Colon, Integer}}})
    @ FillArrays ~/Dropbox/JuliaPackages/FillArrays.jl/src/FillArrays.jl:253

Possible fix, define
  reshape(::AbstractFill, ::Tuple{Union{Colon, Integer}, Vararg{Union{Colon, Integer}}})

After this PR

julia> reshape(Fill(2,6), big(2), :)
2×3 Fill{Int64}, with entries equal to 2
codecov[bot] commented 1 year ago

Codecov Report

Merging #333 (79e70d0) into master (0929e07) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   98.67%   98.67%           
=======================================
  Files           5        5           
  Lines         452      452           
=======================================
  Hits          446      446           
  Misses          6        6           
Impacted Files Coverage Δ
src/OffsetArrays.jl 98.29% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

timholy commented 1 year ago

The only potential problem with that is that Tuple{} <: Tuple{Vararg{T}}. Do we have a test for a 0d reshape?

jishnub commented 1 year ago

Yes, I have added a test for that in this PR. That works because Base defines reshape for Tuple{Vararg{Int}}, which is more specific than Tuple{Vararg{OffsetAxis}}.

timholy commented 1 year ago

Shoot, I missed that. Thanks for doing this! This is subtle design and you've clearly mastered it.

jishnub commented 1 year ago

Test failure on v1.0 is due to https://github.com/JuliaMath/IntegerMathUtils.jl/issues/5, and would hopefully be fixed by https://github.com/JuliaRegistries/General/pull/86557