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

to_index now throws an ArgumentException instead of an UndefVarError … #51

Merged
merged 1 commit into from
Oct 27, 2016

Conversation

ajkeller34
Copy link
Collaborator

…when given repeated axes.

There is a TODO in indexing.jl asking whether or not AxisArrays should allow indexing with repeated axes. This PR does not make a decision about that, but on master the expected ErrorException is not thrown:

julia> using AxisArrays
INFO: Recompiling stale cache file /Users/ajkeller/.julia/lib/v0.5/AxisArrays.ji for module AxisArrays.

julia> A = AxisArray(rand(2,2), :x, :y)
2-dimensional AxisArray{Float64,2,...} with axes:
    :x, Base.OneTo(2)
    :y, Base.OneTo(2)
And data, a 2×2 Array{Float64,2}:
 0.695732   0.261082
 0.0772629  0.286141

julia> A[Axis{:y}(1), Axis{:y}(1)]
ERROR: UndefVarError: y not defined
 in to_index at /Users/ajkeller/.julia/v0.5/AxisArrays/src/indexing.jl:106 [inlined]
 in getindex(::AxisArrays.AxisArray{Float64,2,Array{Float64,2},Tuple{AxisArrays.Axis{:x,Base.OneTo{Int64}},AxisArrays.Axis{:y,Base.OneTo{Int64}}}}, ::AxisArrays.Axis{:y,Int64}, ::AxisArrays.Axis{:y,Int64}) at /Users/ajkeller/.julia/v0.5/AxisArrays/src/indexing.jl:76

In this PR, in addition to displaying the expected error message I've made it an ArgumentError rather than an ErrorException:

 julia> using AxisArrays
INFO: Recompiling stale cache file /Users/ajkeller/.julia/lib/v0.5/AxisArrays.ji for module AxisArrays.

julia> A = AxisArray(rand(2,2), :x, :y)
2-dimensional AxisArray{Float64,2,...} with axes:
    :x, Base.OneTo(2)
    :y, Base.OneTo(2)
And data, a 2×2 Array{Float64,2}:
 0.698423  0.522879
 0.957749  0.178753

julia> A[Axis{:y}(1), Axis{:y}(1)]
ERROR: ArgumentError: multiple indices provided on axis y
 in getindex(::AxisArrays.AxisArray{Float64,2,Array{Float64,2},Tuple{AxisArrays.Axis{:x,Base.OneTo{Int64}},AxisArrays.Axis{:y,Base.OneTo{Int64}}}}, ::AxisArrays.Axis{:y,Int64}, ::AxisArrays.Axis{:y,Int64}) at /Users/ajkeller/.julia/v0.5/AxisArrays/src/indexing.jl:76

I'll keep working my way towards a PR for atvalue as discussed earlier, I'm just trying to understand indexing better first.

@ajkeller34
Copy link
Collaborator Author

(I didn't say it explicitly, but the problem was that the axis name, a symbol, was being interpolated into the expression returned from the generated function, and so error saw the axis name not as a string but as a symbol to look up.)

Copy link
Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! Ok to merge once travis passes.

@ajkeller34
Copy link
Collaborator Author

The travis failure looks unrelated, not sure what happened.

@mbauman
Copy link
Member

mbauman commented Oct 27, 2016

Yup, looks like some sort of precompilation-related error. I restarted the builds and they passed.

@mbauman mbauman merged commit 040f115 into JuliaArrays:master Oct 27, 2016
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

Successfully merging this pull request may close these issues.

None yet

2 participants