Skip to content

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

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

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 12, 2019

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.

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.
Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. 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,))...))

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

@propagate_inbounds getindex_converted(A, idxs::Idx...) = A[idxs...]
@propagate_inbounds setindex!_converted(A, v, idxs::Idx...) = (A[idxs...] = v)

@propagate_inbounds getindex_converted(A, idxs...) = A.data[idxs...]
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to re-wrap the Axes into the result somehow here (if an array is returned) or is this just a case of "AxisArrays doesn't understand the type of the indices, so all bets are off"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. If AxisArrays doesn't know how to handle the index, then I don't think it can make sensible decisions about whether to return the axis wrapper or not. The particular case I was worried about returns a scalar, so of course it's biased to do that. If it should return more then I think one needs some glue code (i.e., via Requires or similar).

@timholy
Copy link
Member Author

timholy commented Aug 13, 2019

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

c42f commented Aug 13, 2019

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

timholy commented Aug 13, 2019

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 😉.

@c42f
Copy link
Member

c42f commented Aug 18, 2019

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

Never mind though, version numbers are cheap :-)

@timholy timholy merged commit b078564 into master Aug 19, 2019
@timholy timholy deleted the teh/unknown_index_types branch August 19, 2019 12:54
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.

2 participants