jonniedie / ComponentArrays.jl

Arrays with arbitrarily nested named components.
MIT License
287 stars 34 forks source link

ComponentArrays introduces ambiguity for Base.dropdims #193

Closed marius311 closed 1 year ago

marius311 commented 1 year ago

ComponentArrays v0.13.8, Julia 1.8.5

julia> dropdims([1;;], dims=(1,2))
0-dimensional Array{Int64, 0}:
1

julia> using ComponentArrays

julia> dropdims([1;;], dims=(1,2))
ERROR: MethodError: reshape(::Matrix{Int64}, ::Tuple{}) is ambiguous. Candidates:
  reshape(A::AbstractArray, axs::Tuple{Vararg{var"#s15", N}} where var"#s15"<:ComponentArrays.CombinedAxis) where N in ComponentArrays at /global/u1/m/marius/.julia/packages/ComponentArrays/NM9IH/src/array_interface.jl:13
  reshape(a::Array{T, M}, dims::Tuple{Vararg{Int64, N}}) where {T, N, M} in Base at reshapedarray.jl:40
  reshape(parent::AbstractArray, dims::Tuple{Vararg{Int64, N}} where N) in Base at reshapedarray.jl:112
  reshape(parent::AbstractArray, dims::Tuple{Vararg{Union{Colon, Int64}}}) in Base at reshapedarray.jl:119
Possible fix, define
  reshape(::Array{T, M}, ::Tuple{}) where {T, M}
jonniedie commented 1 year ago

Oh, yeah it looks like we need to put a single argument of CombinedAxis before the Vararg to avoid type piracy here.

marius311 commented 1 year ago

Maybe this breaks some other thing I'm not aware of since I'm not familiar with the CA code but maybe another (better?) solution is first arg should be ::ComponentArray? Looking at e.g. Base and CUDA their defintions both have a ::NTuple{N,Int} second arg but a concrete first arg?

jonniedie commented 1 year ago

Now that I'm looking back at this, this reshape method was introduced because the reshape method of OffsetArrays commits some type piracy and broke some things, I think. I'm going to just undo that completely and point people toward OffsetArrays when things break.

jonniedie commented 1 year ago

It looks like whatever was broken before isn't broken now, so... 🤷🏼