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

Try converting indexes once and then defer to the parent #164

Closed timholy closed 5 years ago

timholy commented 5 years ago

AxisArrays has to handle many different index types, but it can't handle types defined in other packages. This provides for a fallback mode in which AxisArrays tries to convert "fancy" index types into "regular" ones, but if this fails then it assumes this is something the parent knows how to handle.

Fixes the following error:

julia> using AxisArrays, Interpolations

julia> a = AxisArray(1:3, :x)
1-dimensional AxisArray{Int64,1,...} with axes:
    :x, Base.OneTo(3)
And data, a 3-element UnitRange{Int64}:
 1
 2
 3

julia> itp = interpolate!(a, BSpline(Linear()));

julia> itp(1.2)
ERROR: MethodError: no method matching isless(::Int64, ::Interpolations.WeightedAdjIndex{2,Float64})
Closest candidates are:
  isless(::Missing, ::Any) at missing.jl:66
  isless(::Real, ::AbstractFloat) at operators.jl:157
  isless(::Real, ::Real) at operators.jl:346
  ...
Stacktrace:
 [1] lt(::Base.Order.ForwardOrdering, ::Int64, ::Interpolations.WeightedAdjIndex{2,Float64}) at ./ordering.jl:49
 [2] searchsortedfirst at ./sort.jl:178 [inlined]
 [3] searchsortedfirst at ./sort.jl:291 [inlined]
 [4] #searchsortedfirst#4 at ./sort.jl:293 [inlined]
 [5] searchsortedfirst at ./sort.jl:293 [inlined]
 [6] searchsorted at /home/tim/.julia/dev/AxisArrays/src/search.jl:20 [inlined]
 [7] axisindexes(::Type{AxisArrays.Dimensional}, ::Base.OneTo{Int64}, ::Interpolations.WeightedAdjIndex{2,Float64}) at /home/tim/.julia/dev/AxisArrays/src/indexing.jl:183
 [8] axisindexes at /home/tim/.julia/dev/AxisArrays/src/indexing.jl:174 [inlined]
 [9] macro expansion at /home/tim/.julia/dev/AxisArrays/src/indexing.jl:367 [inlined]
 [10] _to_index at /home/tim/.julia/dev/AxisArrays/src/indexing.jl:323 [inlined]
 [11] to_index at /home/tim/.julia/dev/AxisArrays/src/indexing.jl:320 [inlined]
 [12] getindex at /home/tim/.julia/dev/AxisArrays/src/indexing.jl:117 [inlined]
 [13] (::Interpolations.BSplineInterpolation{Float64,1,AxisArray{Int64,1,UnitRange{Int64},Tuple{Axis{:x,Base.OneTo{Int64}}}},BSpline{Linear},Tuple{Base.OneTo{Int64}}})(::Float64) at /home/tim/.julia/dev/Interpolations/src/b-splines/indexing.jl:8
 [14] top-level scope at REPL[5]:1

I want to give folks a little time to complain about anything this breaks, so I won't merge in the next week. But @ChantalJuntao you can check out this branch.

timholy commented 5 years ago

So the call tree for getindex(A::AxisArray, w::WeightedIndex) (manually inlining) is

getindex(A, w) -> getindex_converted(A, to_index(A,(w,))...) -> getindex(A, to_index(A,(w,))...) # since to_index returns an Idx -> AxisArray(A.data[to_index(A,(w,))...], reaxis(A, to_index(A,(w,))...))

Not quite. to_index doesn't do anything with WeightedIndex (it returns it as-is), and Idx is Union{Colon,Real,AbstractArray{Int}}. So it takes the getindex_converted(A, idxs...) path rather than the getindex_converted(A, idxs::Idx...) path, which means it strips the wrapper.

Do you have an example/test for a kind of specialized index that AxisArrays can't/shouldn't handle?

The test I submitted is one such example. Or is there something that's not covering?

c42f commented 5 years ago

The test I submitted is one such example. Or is there something that's not covering?

Sorry, my mistake not reading closely enough. This looks good to me then.

timholy commented 5 years ago

I was impressed you asked. It's quite difficult to mentally execute dispatch paths as complex as those in this package!

Thanks for the review. I'll leave it open until next Monday; aside from giving others a chance to point out concerns, I'm on vacation and I'd rather not have to deal with fallout from anything I break :wink:.

c42f commented 5 years ago

Ah darn, I forgot about this one or I would have put it in the release.

Never mind though, version numbers are cheap :-)