DO NOT MERGE: rely on julia-0.5 indexing mechanisms to fix a segfault

[timholy]

This fixes JuliaLang/julia#17328, and is one of those happy circumstances where deleting a lot of code makes things work better. By declaring that the number of indices must match the dimensionality of the array, you're now allowing some of the automatic fallback mechanisms to kick in. In particular, trying to index a[i,j] when a is 1-dimensional should just check that j==1 (or is a CartesianIndex{0} or similar) and then return a[i]. By restricting dispatch to the case where the number of indices matches the dimensionality, you can let this happen.

The reason this is marked DO NOT MERGE is that it works only on julia-0.5, and I haven't tested the impact this has on performance (probably favorable due to those @inline statements). On earlier versions of julia, you could annotate all those methods for the specific dimensionality, but I think the "splatting" version is basically unfixable 😦 unless you write the bounds-checking code yourself (which wouldn't be so bad, e.g., map(i->@assert(i==1), I[ndims(a)-5:end]) or something, except you might need to handle 1:1, [1], CartesianIndex types, etc.).

CC @tkelman.

[davidavdav]

Thanks,

yes, I won't immediately merge, because some tests are failing currently, even for 0.5. But I like the simplification, I was never charmed by the many definitions of getindex(). I'll see if I can get it to work.

[timholy]

Just FYI, on travis all the 0.5 tests pass. What are you seeing fail?

[davidavdav]

You are right---I noticed the Travis stats. Turns out I was running a 6-day old master. Didn't realize it was this much bleeding edge.

[timholy]

Hmm, I'm surprised; I thought this would have passed ever since the merger of JuliaLang/julia#11242, which was back in early May.

[davidavdav]

I still have the error in my scroll-back history, this is what I got with Commit f53b00e

prondavid NamedArrays.jl (timholy-teh/simpler_indexing)$ julia-0.5 runtest.jl 
Starting test, no assertions should fail... construction, getindex, copy, setindex, sum, conversions, changing names, multi-dimensional, dodgy indices, Error During Test
  Test threw an exception of type MethodError
  Expression: m[[1 // 4,1 // 3]] == m.array[[4,3]]
  MethodError: getindex(::NamedArrays.NamedArray{Float64,1,Array{Float64,1},Tuple{Dict{Rational{Int64},Int64}}}, ::CartesianIndex{1}) is ambiguous. Candidates:
    getindex(a::NamedArrays.NamedArray, it::CartesianIndex) at /Users/david/werk/julia/NamedArrays.jl/src/index.jl:18
    getindex{T,N}(a::NamedArrays.NamedArray{T,N,AT<:Any,DT<:Any}, I::Vararg{Any,N}) at /Users/david/werk/julia/NamedArrays.jl/src/index.jl:13
   in next at ./iterator.jl:0 [inlined]
   in ==(::NamedArrays.NamedArray{Float64,1,Array{Float64,1},Tuple{Dict{Rational{Int64},Int64}}}, ::Array{Float64,1}) at ./abstractarray.jl:1247
   in eval_comparison(::Expr) at ./test.jl:169
   in include_from_node1(::String) at ./loading.jl:426 (repeats 3 times)
   in process_options(::Base.JLOptions) at ./client.jl:266
   in _start() at ./client.jl:322
...

an ambiguity (how much I love those) which is in the "dodgy indices" test, where admittedly dodgy indices are used. The update of julia went Updating f53b00e..0b98dbe, and after that the test passed.

[timholy]

The PR doesn't rely on anything all that new, so I think you just got caught by a recent breakage. Perhaps JuliaLang/julia#17350. Anyway, not a big deal either way, except of course my sympathies for the various bugs.

[chipkent]

Has this fix been tested with the 0.5.0 release candidates?

[chipkent]

Bumping this item since Julia 0.5 has now been out for a while.

[timholy]

Worth noting that AxisArrays is out now.

[timholy]

@davidavdav, if you want this, do you want to rebase it or are you hoping I will? Or are you reluctant to abandon 0.4?

[davidavdav]

Hi, @timholy,

My policy is to maintain 0.4 and 0.5 until 0.6/1.0. (Just recently I spent a day porting/debugging production code to go from julia-0.4 to 0.5.)

However, addressing #40 probably benefits from this, so I will first version-dependent merge this then.