Generalized indices and arbitrary indexing support. #1
Conversation
| # Ambiguity resolution with Base | ||
| @inline Base.similar(arr::CircularArray, ::Type{T}, dims::Tuple{Int64,Vararg{Int64}}) where T = _similar(arr,T,dims) | ||
| # Ambiguity resolution with OffsetArrays, for the case similar(arr) where arr.data::OffsetArray | ||
| @inline Base.similar(arr::CircularArray, ::Type{T}, dims::Tuple{OffsetAxis, Vararg{OffsetAxis}}) where T = _similar(arr,T,dims) |
There was a problem hiding this comment.
Is the hard dependency on OffsetArrays for this really necessary? I doubt that it would have to be. Why would this ambiguity resolution be needed at all? I'd like to avoid this hard dependency if possible, it's fine using the package for testing though.
There was a problem hiding this comment.
The reason ambiguity resolution is needed is the similar method mentioned here: JuliaArrays/OffsetArrays.jl#87.
Without changes to Base or OffsetArrays, the only solutions I can see are:
- a dependency on
OffsetArrays - copy-pasting the
OffsetAxisdefinition fromOffsetArrays - defining our own axis type as suggested here Type-piracy in similar with
OffsetAxisJuliaArrays/OffsetArrays.jl#87 (comment), and definingsimilarjust for those. This would meanaxesconverts the underlying arraysaxesto a custom range type, andsimilardoes the opposite conversion to send the axes to the underlying array. This would solve the casesimilar(a::CircularArray)wherea.datais offset, but not a direct call from user code likesimilar(a::CircularArray, -2:2, -2:2)(which would dispatch on theOffsetArraysdefinition and thus fail to produce a circular array). Perhaps I'm misunderstanding how this mechanism should be used. Maybe @timholy can elucidate? EDIT: actually, I tried this now and a lot of tests fail because indexing operations invokesimilarwith the index's axes, which sinks this whole approach.
Otherwise, I think changing Base and OffsetArrays as suggested here JuliaArrays/OffsetArrays.jl#87 (comment) would solve this problem too.
There was a problem hiding this comment.
This sounds like an issue that needs to be fixed in OffsetArrays. I am not a fan of adding workarounds for problems in other packages in here.
There was a problem hiding this comment.
If it's absolutely necessary, I'd rather copy the OffsetAxis definition over. It's just a tuple, after all.
There was a problem hiding this comment.
JuliaArrays/OffsetArrays.jl#90 shows an example of defining your own axis type. You can specialize axes for the axes, as done in that PR.
There was a problem hiding this comment.
I may be missing something, but as far as I can see, this OffsetArrays example works because it defines similar not only for its custom index types, but also for "standard" types appearing in the OffsetAxis union, because getindex(A,I...) generally calls similar with I's axes. We similarly can't avoid defining similar on the "standard" types, and then we're back with the ambiguity errors. If I only define similar for a custom axis type, then indexing ::CircularArray[::UnitRange] fails to produce a CircularArray (because it calls a similar method from base).
At this point, I don't see how a custom index type is helpful at all. For now I will resolve the ambiguity as in BlockArrays.jl.
|
I have replaced the |
| @inline Base.checkbounds(::CircularArray, _...) = nothing | ||
|
|
||
| @inline _similar(arr::CircularArray, ::Type{T}, dims) where T = CircularArray(similar(arr.data,T,dims)) | ||
| @inline Base.similar(arr::CircularArray, ::Type{T}, dims::Tuple{Base.DimOrInd, Vararg{Base.DimOrInd}}) where T = _similar(arr,T,dims) |
There was a problem hiding this comment.
After some more testing, I noticed that this line does not seem to be covered by the tests. Do you happen to have an example for where it might be used? Or did the addition of the ambiguity resolution for OffsetArrays cover this? Technically, this method is more broad than the one below since DimOrInd is more generic, but I cannot think of anything that would ever use this, short of someone implementing their own axis type, which would once again trigger the same ambiguity as below anyway.
There was a problem hiding this comment.
Yes, I added this before the ambiguity resolution.
I think you're right, and we will face an ambiguity error on similar when indexing with arrays defining a custom axis type in the "standard" way defined in the Julia docs. I tested this with CatIndices.BidirectionalVector, and indeed indexing c[bidi_vec] where c is a CircularArray triggers the ambiguity. If this more general similar method is removed the ambiguity disappears, but the result of indexing is no longer circular, which is inconsistent with c[a::Array].
I think maintaining the circularity in, e.g. c[1:5] is really useful, and it's unfortunate if we can't have it for c[i] where i has custom axes, but I'm not sure how it would be possible with how array indexing is designed in Base.
There was a problem hiding this comment.
I guess I will keep this as it is for now. Thank you again for the contribution.
Adds support for generalized (non-1-based) axes of the underlying array, as well as generalized indexing (e.g. by ranges or logical arrays).