Add Tuple(::CartesianIndex)

[martinholters]

Make iterating a CartesianIndex equivalent to iterating the tuple it holds, being consistent with its indexing behavior. This is especially useful for destructuring as in e.g. i, j = indmax(A).

Just add Tuple(::CartesianIndex) as per discussion below. Destructuring now would require i, j = Tuple(indmax(A)), but it's still possible without explicitly accessing the I field.

[timholy]

Historically I've opposed this change simply as a means of avoiding a major (and unfortunately very natural) performance trap:

julia> Base.eltype(index::CartesianIndex) = eltype(index.I)

julia> Base.start(index::CartesianIndex) = start(index.I)

julia> Base.next(index::CartesianIndex, state) = next(index.I, state)

julia> Base.done(index::CartesianIndex, state) = done(index.I, state)

julia> function mysum1(A)
           s = 0.0
           @inbounds for i in CartesianRange(indices(A))
               s += A[i]
           end
           s
       end
mysum1 (generic function with 1 method)

julia> function mysum2(A)
           s = 0.0
           @inbounds for i in CartesianRange(indices(A))
               s += A[i...]
           end
           s
       end
mysum2 (generic function with 1 method)

julia> A = rand(10,10,10,10,10,10);

# after warmup (@btime is broken on master)
julia> @time mysum1(A)
  0.003628 seconds (5 allocations: 176 bytes)
500411.4240107683

julia> @time mysum2(A)
  1.641540 seconds (15.00 M allocations: 686.646 MiB, 5.57% gc time)
500411.4240107683

However, given that a version using A[i.I...] is fast, perhaps there could be accompanying changes to inference.jl that would eliminate this penalty.

[martinholters]

Ah, yes, that's a bad trap. Though possible, I'd rather not add a special case/hack to inference. The much nicer and more general approach would be to add sufficient constant propagation to figure out cases where the length of the object to be splatted is known at compile time and then do the same inlining as for Tuples. Clearly, that is beyond the scope of this PR.

So for now, I guess we'd have to decide between convenience and avoiding a performance pitfall.

[mbauman]

Could just defining Tuple be sufficient for your purposes?

julia> Tuple(I::CartesianIndex) = I.I
Tuple

julia> function mysum2(A)
                  s = 0.0
                  @inbounds for i in CartesianRange(indices(A))
                      s += A[Tuple(i)...]
                  end
                  s
              end
mysum2 (generic function with 1 method)

julia> A = rand(10,10,10,10,10,10);

julia> @time mysum2(A)
  0.625158 seconds (222.97 k allocations: 12.000 MiB, 8.90% gc time)
500281.72646593733

julia> @time mysum2(A)
  0.008289 seconds (5 allocations: 176 bytes)
500281.72646593733

Edit: I suppose it doesn't solve the destructuring problem. :-\

[timholy]

I guess my viewpoint is that since it's easy to destructure with i.I, then in the absence of improvements to inference.jl the status quo is preferable. It's just too easy to fall into that performance trap and then think the whole CartesianIndex thing is useless.

[martinholters]

So would we advertise i.I for accessing the internal tuple for destructuring, or do we view that as an implementation detail and introduce the Tuple(::CartesianIndex) method as a means for accessing it?

[timholy]

I like the Tuple version better.

[martinholters]

Ok, changed the PR to only add the Tuple method instead of adding iteration capabilities.