Type-piracy in similar with `OffsetAxis`

[dlfivefifty]

This is type-piracy:

OffsetArrays.jl/src/OffsetArrays.jl

Line 99 in d50463f

function Base.similar(A::AbstractArray, ::Type{T}, inds::Tuple{OffsetAxis,Vararg{OffsetAxis}}) where T

[timholy]

It's basically "sanctioned type piracy." The issue is that Base is incapable of creating arrays with axes that start at anything other than 1. The dispatch hierarchy for Base ensures that any combination of inputs that can be reduced to axes that start at 1 will get caught by methods in Base. This method is the fallback, in case that doesn't happen. That's OK here because this is the foundational package for arrays with offset axes.

Any other package that wants to define offset axes has to define their own axis types. See https://docs.julialang.org/en/latest/devdocs/offset-arrays/#Custom-AbstractUnitRange-types-1.

Does that cover it? Or is there an issue that I'm not understanding?

[dlfivefifty]

The issue is that as designed now type ambiguities in other packages (in this case BlockArrays.jl) only are triggered if a user loads OffsetArrays.jl.

I think a better approach would be in Base to add something like:

_offset_similar(_) = error("Not implemented in Base. Please use external package.")
Base.similar(A::AbstractArray, ::Type{T}, inds::Tuple{OffsetAxis,Vararg{OffsetAxis}}) where T = _offset_similar(A, T, index)

then OffsetArrays.jl would overload Base._offset_similar. This means ambiguity is consistent.

[timholy]

If you can sync this across Base, BlockArrays, and OffsetArrays, by all means go for it! Might want to also check CatIndices.jl as an example of a package that defines its own index types.

[dlfivefifty]

I won’t be able to tackle this right now but perhaps in the future. Maybe just leave this open for now?

[yha]

This type piracy has become worse with #90: now similar is pirated for general AbstractArray with the new index type IdOffsetRange, which means other packages defining similar(a::OtherArrayType,...) must disambiguate on an index type internal to OffsetArrays. See Vexatos/CircularArrays.jl#2 (comment).
I think what is needed is something like #87 (comment) suggested above but with OffsetAxis being an abstract type that packages can subtype. Then the rule would be that package may specialize offset_similar only for the last (indexes) argument, and similar only for the first (array) argument, like this:

# In Base
const OffsetDims = Tuple{OffsetAxis,Vararg{OffsetAxis}}
similar(A::AbstractArray, ::Type{T}, off::OffsetDims) where T = offset_similar(A, T, off)

# In OffsetArray
struct IdOffsetRange <: OffsetAxis
  ...
end
function Base.offset_similar(A::AbstractArray, ::Type{T}, inds::IdOffsetRange) where T
  ...
  OffsetArray(...)
end

# In another package
Base.similar(A::MyArrayType, ::Type{T}, dims::Dims) = MyArrayType(...)
Base.similar(A::MyArrayType, ::Type{T}, inds::OffsetDims) = MyArrayType(offset_similar(A,T,inds))
# and/or
Base.offset_similar(A::AbstractArray, ::Type{T}, inds::MyOffsetDimsType) = MyArrayType(...)

[timholy]

Base ranges need an overhaul anyway to address JuliaLang/julia#30950. That PR was starting to turn into a nightmare, I think the best option will be to start fresh and go slowly, thinking very carefully about how to avoid breakage. (There is a chance that those issues simply can't be fixed until Julia 2.0 😦 ). It would be good to address this as part of that effort.

I don't see myself as having time for it in the next few weeks or more, so definitely looking for help from others!

[Tokazama]

I'm working on something similar to this right now (OffsetAxes.jl) that builds on AxisIndices.jl. If I understand correctly (which it's possible I don't) then similar was originally designed to handle element types and size. This inserts the notion of "axes" which isn't really a part of Array and may not be applicable to many other subtypes of AbstractArray. I'm running into some similar things with reshape too.

Would it make sense to have a more explicit argument for what is happening here? Perhaps something like reaxes because the point is to change the axes of an array.

[yha]

I think the abmiguity issues caused by this piracy is nearly resolved now. At least, it's resolved for CircularArrays, where this issue previously caused trouble (see Vexatos/CircularArrays.jl#8)
The reason is that the signature has become

OffsetArrays.jl/src/OffsetArrays.jl

Line 219 in f20ecfc

function Base.similar(A::AbstractArray, ::Type{T}, inds::Tuple{OffsetAxisKnownLength,Vararg{OffsetAxisKnownLength}}) where T

and #157 changed OffsetAxisKnownLength to mean Union{Integer, AbstractUnitRange} (which is the same thing as Base.DimOrInd).
So packages can define similar for dims::Tuple{DimOrInd, Vararg{DimOrInd}} without triggering ambiguity with this method and without depending on a type internal to OffsetArrays. This signature is almost the current "official" recommendation in
https://github.com/JuliaLang/julia/blob/110f1256ac88ee0f12885289e807229acf9c44a3/base/abstractarray.jl#L723-L727 (it says UnitRange rather than AbstractUnitRange).
Perhaps the official documented recommendation should now be to use Base.DimOrInd when specializing similar for offset arrays.

[akosuas]

I just encountered something similar when trying to reshape an Array where the length was given as Int32 instead of Int64:

julia> using OffsetArrays
julia> which(Base.reshape, (Array{Float64, 1}, Int32))
reshape(A::AbstractArray, inds::Union{Colon, Integer, AbstractUnitRange}...) in OffsetArrays at ~/.julia/packages/OffsetArrays/PXUn7/src/OffsetArrays.jl:225
(MWE) pkg> st
Project MWE v0.1.0
Status `MWE/Project.toml`
  [6fe1bfb0] OffsetArrays v1.4.1

This feels a bit naughty to me.

[mcabbott]

To be fair, that's a MethodError without this package, and the expected answer with it. More surprising is:

julia> reshape(rand(1,2), Int32(2))
2-element Vector{Float64}:
 0.23063257026910478
 0.007514586985956306

julia> reshape(rand(2,3), :, Int32(2))
ERROR: StackOverflowError:
Stacktrace:
 [1] reshape(A::Matrix{Float64}, inds::Tuple{Colon, Int32})
   @ OffsetArrays ~/.julia/packages/OffsetArrays/ExQCD/src/OffsetArrays.jl:227