JuliaArrays / AxisArrays.jl

Performant arrays where each dimension can have a named axis with values
http://JuliaArrays.github.io/AxisArrays.jl/latest/
Other
200 stars 41 forks source link

confusing behavior with integer indices #117

Closed mlubin closed 7 years ago

mlubin commented 7 years ago

Using AxisArrays master:

julia> x = AxisArray(Array{Int}(3), Axis{:t}(-3:-1))
1-dimensional AxisArray{Int64,1,...} with axes:
    :t, -3:-1
And data, a 3-element Array{Int64,1}:
 0
 0
 0

julia> x[-3]
ERROR: BoundsError: attempt to access 3-element Array{Int64,1} at index [-3]
Stacktrace:
 [1] getindex(::AxisArrays.AxisArray{Int64,1,Array{Int64,1},Tuple{AxisArrays.Axis{:t,UnitRange{Int64}}}}, ::Int64) at /home/mlubin/.julia/v0.6/AxisArrays/src/indexing.jl:41

The corresponding definition of getindex is:

# Simple scalar indexing where we just set or return scalars
@propagate_inbounds Base.getindex(A::AxisArray, idxs::Int...) = A.data[idxs...]

Is this a bug or a feature? Are integer-indexed axes like -3:-1 not really supported?

iamed2 commented 7 years ago

Integer indices are assumed to be indices in the original array. This could conceivably change (see https://github.com/JuliaArrays/AxisArrays.jl/issues/84).

To force value indexing you can use atvalue, e.g.:

julia> x = AxisArray(Array{Int}(3), Axis{:t}(-3:-1))
1-dimensional AxisArray{Int64,1,...} with axes:
    :t, -3:-1
And data, a 3-element Array{Int64,1}:
 4654596688
 4388655056
 4387773968

julia> x[atvalue(-3)]
4654596688
mlubin commented 7 years ago

Ok, looks like this is a duplicate of https://github.com/JuliaArrays/AxisArrays.jl/issues/84. This issue is going to block us from using AxisArrays in JuMP. Would you be open to a PR changing this behavior?

iamed2 commented 7 years ago

I (and Andreas, it looks like) would personally appreciate the consistency! Owners (Matt/Tim/Andrew) would be the ones reviewing though; I don't really know their feelings on this. I think always using value indexing would be a good way to make the interface simpler and more consistent, which I know everyone would like.

timholy commented 7 years ago

I think the only way one could pull this off is to also incorporate #81. This is pretty scary territory, but it just might be the only sensible way forward.

@mlubin, if you just want to index with arbitrary integer indices, OffsetArrays is already available. Be sure you read https://docs.julialang.org/en/latest/devdocs/offset-arrays/#Arrays-with-custom-indices-1. I'm intending to "take off the training wheels," aka re-support size, length, and @inbounds, pretty soon (once other deadlines aren't a raging inferno).

mlubin commented 7 years ago

@timholy, thanks for the response. OffsetArrays are not sufficient for us. In JuMP we currently have a custom container type (JuMPArray) that is like an AxisArray but with no special case for integer indices. We'd like to replace JuMPArray with AxisArray because it's much better thought-out than our custom container.

However, if we move forward with this, AxisArrays need to be a drop-in replacement for basic use cases because we're returning this container directly to JuMP users. It doesn't make sense for me to ask JuMP users to suddenly change their code to use atvalue with integer indices or start wrapping indexing operations with a macro (https://github.com/JuliaArrays/AxisArrays.jl/pull/118).

I don't mean to come in and start imposing changes here if the current behavior is what really makes sense for the intended use of AxisArrays.

timholy commented 7 years ago

I will defer to others to comment here. I should really sit down and spend an hour figuring out #81 and its implications, but I don't have that hour right now. Sorry.