Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

confusing behavior with integer indices #117

Closed
mlubin opened this issue Sep 10, 2017 · 6 comments
Closed

confusing behavior with integer indices #117

mlubin opened this issue Sep 10, 2017 · 6 comments

Comments

@mlubin
Copy link

mlubin commented Sep 10, 2017

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
Copy link
Collaborator

iamed2 commented Sep 10, 2017

Integer indices are assumed to be indices in the original array. This could conceivably change (see #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
Copy link
Author

mlubin commented Sep 10, 2017

Ok, looks like this is a duplicate of #84. This issue is going to block us from using AxisArrays in JuMP. Would you be open to a PR changing this behavior?

@mlubin mlubin closed this as completed Sep 10, 2017
@iamed2
Copy link
Collaborator

iamed2 commented Sep 10, 2017

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.

mlubin added a commit to mlubin/AxisArrays.jl that referenced this issue Sep 11, 2017

Verified

This commit was signed with the committer’s verified signature.
driesvints Dries Vints
@timholy
Copy link
Member

timholy commented Sep 11, 2017

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
Copy link
Author

mlubin commented Sep 11, 2017

@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 (#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
Copy link
Member

timholy commented Sep 12, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants