Skip to content
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

Deprecate +(::CartesianIndex, ::Number) #26227

Closed
dlfivefifty opened this issue Feb 27, 2018 · 14 comments · Fixed by #26284
Closed

Deprecate +(::CartesianIndex, ::Number) #26227

dlfivefifty opened this issue Feb 27, 2018 · 14 comments · Fixed by #26284
Assignees
Labels
deprecation This change introduces or involves a deprecation
Milestone

Comments

@dlfivefifty
Copy link
Contributor

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

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

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

@martinholters
Copy link
Member

That use does not justify the method, IMO.

@mbauman
Copy link
Member

mbauman commented Feb 27, 2018

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

@mbauman mbauman added the deprecation This change introduces or involves a deprecation label Feb 27, 2018
@dlfivefifty
Copy link
Contributor Author

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 JeffBezanson added the triage This should be discussed on a triage call label Feb 28, 2018
@mbauman mbauman self-assigned this Mar 1, 2018
@mbauman mbauman added this to the 1.0 milestone Mar 1, 2018
@JeffBezanson
Copy link
Member

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

@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Mar 1, 2018
mbauman added a commit that referenced this issue Mar 1, 2018
mbauman added a commit that referenced this issue Mar 1, 2018
@mbauman
Copy link
Member

mbauman commented Mar 1, 2018

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

timholy commented Sep 30, 2018

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

dlfivefifty commented Sep 30, 2018

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

Edit: Sorry, just saw mbauman's explanation.

@timholy
Copy link
Member

timholy commented Sep 30, 2018

#26227 (comment). 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
Copy link
Contributor Author

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

timholy commented Sep 30, 2018

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

@dlfivefifty
Copy link
Contributor Author

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 JuliaArrays/FillArrays.jl#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
Copy link
Member

timholy commented Sep 30, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants