Julia: Deprecate +(::CartesianIndex, ::Number)

[dlfivefifty]

This seems inconsistent:

julia> CartesianIndex((1,2)) + 1 # NO deprecation warning
CartesianIndex(2, 3)

julia> [1,2] + 1 # deprecation warning
┌ Warning: `a::AbstractArray + b::Number` is deprecated, use `broadcast(+, a, b)` instead.
│   caller = top-level scope
└ @ Core :0
2-element Array{Int64,1}:
 2
 3

[martinholters]

Yes, that seems hard to justify. One is tempted to deprecate this to CartesianIndex((1,2)) .+ 1, BUT broadcast at the moment treats CartesianIndex as a scalar, thus just falls back to CartesianIndex((1,2)) + 1 again.

What is this used for, anyway? Could we just deprecate it to its definition CartesianIndex{N}(map(x->x+i, index.I))?

[dlfivefifty]

It's used in start(iter::CartesianIndices)

[martinholters]

That use does not justify the method, IMO.

[mbauman]

Yes, this seems like a good idea. I would bet that some folks might expect CartesianIndex(3, 4) + 1 to return CartesianIndex(4, 4)… or to wrap to the next column as appropriate. Of course, it cannot know that, so we should make folks choose between + CartesianIndex(1,1) and + CartesianIndex(1, 0).

A slightly simpler deprecation would be

+(a::CartesianIndex, i::Int) = a + i*one(a).

[dlfivefifty]

Argument in favour of CartesianIndex((1,2)) .+ 1 being supported: (1) convenience and (2) indexing is supported

julia> CartesianIndex((1,2))[1]
1

Argument against: collect is not supported

julia> CartesianIndex((1,2))|>collect
ERROR: iteration is deliberately unsupported for CartesianIndex. Use `I` rather than `I...`, or use `Tuple(I)...`
Stacktrace:
 [1] error at ./error.jl:33 [inlined]
 [2] start at ./multidimensional.jl:157 [inlined]
 [3] copyto! at ./abstractarray.jl:611 [inlined]
 [4] _collect at ./array.jl:481 [inlined]
 [5] collect at ./array.jl:475 [inlined]
 [6] |>(::CartesianIndex{2}, ::typeof(collect)) at ./operators.jl:744
 [7] top-level scope

I don't have a strong feeling. (I came across it because I have code in BlockArrays.jl that mimicks CartesianRange.)

[JeffBezanson]

I like @mbauman 's suggestion above. Would be good to get @timholy 's opinion though.

[mbauman]

I don't want to use broadcasting here precisely because CartesianIndexes are scalar indices. That's also a great reason to keep them from iterating. For example, I would expect that getindex.((A,), CartesianIndex((1,2)), [3,4,5]) is effectively A[1,2,3:5] (and it is).

[timholy]

Sorry I missed this discussion when real life intruded. The ability to add scalars is most often used when writing multidimensional algorithms, for example the boxcar3 example in https://julialang.org/blog/2016/02/iteration.

TBH I am not entirely convinced we needed to deprecate this: since a CartesianIndex is not an iterable object, deprecating [1,2] + 1 does not logically require that we deprecate CartesianIndex(1,2) + 1. I think of CartesianIndex as "tuples with extra properties that cause them to act as a chunk": this was always one of those extra properties in which it acts as a chunk. I agree with @mbauman that there was some ambiguity, however, in that one might imagine this operation should be the equivalent of "move to the next iterated item." (It's not and can't be, since you also need the CartesianIndices object to determine whether you need to wrap around, but that's a bit subtle.)

If we want to keep this deprecation, #26284 is not bad though it probably should have been oneunit instead of one. Alternatively, what do people think about abusing UniformScaling? The plus is compact syntax, the negative is that it makes their "I am not a vector" status murky.

[dlfivefifty]

What's wrong with CartesianIndex(1,2) .+ 1?

Edit: Sorry, just saw mbauman's explanation.

[timholy]

https://github.com/JuliaLang/julia/issues/26227#issuecomment-369742126. Though perhaps we could add a specially-dispatching version, i.e.

Broadcast.broadcasted(::typeof(+), I::CartesianIndex, i::Integer) = CartesianIndex(Tuple(I) .+ i)

and thus avoid "poisoning" them for general use in broadcasting.

[dlfivefifty]

I’ve been using BroadcaastStyle(styleA, styleB) to indicate a priority of broadcasting: in ApproxFun there’s a FunStyle where broadcasting works point wise, but AbstractArrayStyle takes priority.

Perhaps this system could work here: there’d be a CartesianIndexStyle so the CartesianIndex(1,2) .+ 1 works as proposed but DefaultArrayStyle{1} takes precedence so mbaumans example works as he wants it to.

[timholy]

BroadcastStyle is a trait system for resolving priority in deciding the output-container type, not the behavior of individual arguments to broadcast.

[dlfivefifty]

Hmm, the docs are imprecise about that point. It is also wrong for Sparse arrays: the behaviour of (_ -> randn()).(A) is different depending on whether A is a sparse or not (see https://github.com/JuliaArrays/FillArrays.jl/issues/29)

Where it comes up is that norm.([f; g]) where f and g are Funs has two possible meanings: (1) return [norm(f),norm(g)] or (2) return Fun(x->[norm(f),norm(g)]). I’ve been using precedence to support both: (1) [f,g ] isa Vector{<:Fun} so broadcasting is as expected (2) [f; g] isa Fun{<:VectorSpace} so broadcasting is FunStyle and is done pointwise. But if you mix Vector{<:Fun} and Fun{<:VectorSpace} I’ve been using BroadcastStyle to determine the precedence for different behaviour.

[timholy]

Hmm, the docs are imprecise about that point.

I agree; perhaps I'm speaking more from my original intent than what it evolved into. I'm not certain it's not actually true in practice for Base and its stdlibs, but I agree that the docs make me wonder if I overstated.

It is also wrong for Sparse arrays: the behaviour of (_ -> randn()).(A) is different depending on whether A is a sparse or not (see JuliaArrays/FillArrays.jl#29)

I agree that behavior depends on the arguments but I don't think that's really a consequence of the BroadcastStyle. In particular the behavior is basically determined by this method for which BroadcastStyle has already been "consumed"; this method is pretty much the same form it was before we ever had BroadcastStyle.

I'd say this is different: you're advocating using BroadcastStyle so that a given argument behaves as a scalar or a container depending on the presence of other arguments to broadcast. That seems more drastic than anything else I'm aware of, though I may be overlooking something.