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

What to do about multivalue setindex!? #24086

Open
mbauman opened this issue Oct 10, 2017 · 6 comments
Open

What to do about multivalue setindex!? #24086

mbauman opened this issue Oct 10, 2017 · 6 comments
Labels
domain:arrays [a, r, r, a, y, s] domain:broadcast Applying a function over a collection kind:breaking This change will break code

Comments

@mbauman
Copy link
Sponsor Member

mbauman commented Oct 10, 2017

Problem

We have two ways to assign many values to many locations in an array:

A[2:3] = 4:5
A[2:3] .= 4:5

In this simple case, both of these do the same thing for any array: assign A[2] = 4 and A[3] = 5. But in more complicated cases, both syntaxes are quirky in different ways:

setindex!: A[I...] = X

X must have the same number of elements as the indices I select. Shapes are compared, but singleton dimensions are ignored and the final dimension of X is allowed to linearly span multiple indices. If X is not a subtype of AbstractArray, then the same value is simply broadcast to all indices. Examples:

A = rand(4,1,3,2);
A[:,:,:,:] = 1:24 # Linear spanning
A[:,:,:,:] = reshape(1:24, 4,3,2) # Ignoring singleton dimensions in I
A[:,:,:,:] = reshape(1:24, 4,3,1,2) # Ignoring singleton dimensions in both X and I
A[:,:,:,:] = reshape(1:24, 4,6) # Ignoring singleton dimensions in I and linear spanning
A[:,:,:,:] = 1 # fills A with ones

Now, if I only selects exactly one index then this syntax always puts X into that location:

julia> A = Any[1,2,3];
       A[1] = 4:5;
       A
3-element Array{Any,1}:
  4:5
 2
 3

This is quirky when you actually want to generically assign a single value to multiple locations… and you don't care about its type! Ref. #22747.

broadcast: A[I...] .= X

Broadcasting dictates that the shape of the indexing expression and the shape of X have exactly the same shape if they have the same number of elements. Absolutely no linear spanning is allowed. All of the setindex! examples above are errors except for the scalar 1 case. At the same time, though, .= adds the full set of broadcast features, allowing partial arrays to broadcast across the complete set of indices.

Now, if I only selects exactly one index, then this syntax is quirky:

julia> A = [[1,2,3],4:5,6]
       A[1] .= 0
       A
3-element Array{Any,1}:
 0
  4:5
 6

julia> A = [[1,2,3],4:5]
       A[1] .= 0
       A
2-element Array{AbstractArray{Int64,1},1}:
 [0, 0, 0]
 4:5

This special-casing on the element type is attempting to work around awkwardness due to the scalar/nonscalar distinction in number of indices. That is, A[I] .= 0 modifies A if I is nonscalar, but it should modify the element if I is scalar.

Proposed solution

  1. Deprecate multivalue scalar broadcasting within setindex! in favor of the .= broadcasting syntax. .= is a very nice, explicit signal that you want to broadcast the values on the RHS. The major issue with this deprecation is all the shape permutations that setindex! supports; I believe the deprecation would be:

    `A[I...] = X` is deprecated. Use `A[I...] .= reshape(X, indices(view(A, I...)))` instead

    This is most painful in the common case where X is a one-dimensional linear span over a set of indices that is not linear.

    After attempting to deprecate multivalue scalar indexing in RFC: Deprecate multi-value non-scalar indexed assignment #24368, we realized that it made way more sense to instead deprecate the scalar broadcasting behaviors of setindex. This was done in RFC: Deprecate implicit scalar broadcasting in setindex! #26347.

  2. Remove the eltype special-casing in dotview. If the indices are scalar, then we should always attempt to modify the element. This is breaking deprecatable. Deprecate array of arrays special casing in .= #24095

  3. In 1.x, we could potentially introduce the syntaxes A.[I] and A.[I] .= X. Those are errors at the moment, so we can choose their exact behaviors later. But, they'd allow us to fully detangle the scalar/nonscalar behaviors if we wished:

    • A[i] = x: Always sets x at the scalar index i.
    • A[i] .= X: Always broadcasts X across the element at A[i] (always mutating A[i]). Note that this doesn't make much sense for nonscalar I as mutating A[I] is mutating an unobservable temporary. We would't be able to change this behavior in 1.x, but we could deprecate it in preparation for 2.0.
    • A.[I] = x: Assigns x to every index in I.
    • A.[I] .= X: Always broadcasts X across the indices selected by I (always mutating A).
@mbauman mbauman added kind:breaking This change will break code needs decision A decision on this change is needed status:triage This should be discussed on a triage call domain:broadcast Applying a function over a collection labels Oct 10, 2017
@JeffBezanson JeffBezanson added the domain:arrays [a, r, r, a, y, s] label Oct 10, 2017
@JeffBezanson
Copy link
Sponsor Member

Yes, picking whether to repeat or broadcast in these cases has always been a bit of a wart.

Remove the eltype special-casing in dotview.

💯

@StefanKarpinski
Copy link
Sponsor Member

Note that this doesn't make much sense for nonscalar I as mutating A[I] is mutating an unobservable temporary. We would't be able to change this behavior in 1.x, but we could deprecate it in preparation for 2.0.

Could we just make this an error for non-scalar I to avoid that operation causing confusion?

@mbauman
Copy link
Sponsor Member Author

mbauman commented Oct 12, 2017

Notes from triage:

We found it helpful to fully define what the different syntaxes here would possibly mean. Note that this needn't be the same as which behaviors we choose to support, and we particularly don't need to have this all defined by 1.0. Here's what we thought made sense — it's perhaps most clear with an example where the keys aren't just integers:

d = Dict(1=>1, 2=>[2], 3=>3, 1:3=>[1,2,3])
d[1] = 2        # Sets 1=>2 in d
d[1:3] = 2:4    # Sets 1:3=>2:4 in d
d[2] .= 3       # Modifies value at key 2 to be [3]
d[1:3] .+= 1    # Modifies value at key 1:3 to be [2,3,4]
d.[1:3] = 1     # Sets 1=>1, 2=>1, 3=>1 in d
d.[1:3] .= 2:4  # Sets 1=>2, 2=>3, 3=>4 in d
d.[1:3] = [1,2] # Sets 1=>x, 2=>x, 3=>x in d, with x=[1,2] (all the same object)
d.[1:3] .= 2    # Sets 1=>2, 2=>2, 3=>2 in d

In other words, we can take the same list in bullet point 3 from my first post and just look at it the other way. It's determining which portions of the syntax you're treating as scalar elements:

  • A[I,J] = X: all I, J and X are interpreted as scalars
  • A[I,J] .= X: I and J are interpreted as scalar, X participates in broadcast
  • A.[I,J] = X: X is interpreted as scalar, I and J participate in broadcast (probably APL-like, with J reshaped into a higher dimension than I)
  • A.[I,J] .= X: all I, J, and X participate in broadcast

So, back to the present:

With the current (no-dot) setindex! syntax for arrays, we don't have any ambiguity in terms of whether an index is intended to be used as a scalar or nonscalar element. We do, however, have an ambiguity on the RHS — should it behave like a scalar and get repeated to every element? Or should it participate in some form of broadcast?

So, let's deprecate that ambiguity. That's point 1 in the above proposed solution. That will allow us to layer in the above semantics if we want them (and as we get to them) as new features in 1.x. This has the further advantage of getting rid of another place we we do weird partial linear indexing stuff.

We just need to @deprecate setindex!(A::AbstractArray, X::AbstractArray, I...) A[I...] .= X in those cases where I... selects more than one location. Right now if I... is entirely scalar, then X is treated as a scalar element — exactly the behavior we want.

@mbauman mbauman removed needs decision A decision on this change is needed status:triage This should be discussed on a triage call labels Oct 12, 2017
@mbauman mbauman added this to the 1.0 milestone Oct 12, 2017
@andyferris
Copy link
Member

andyferris commented Oct 12, 2017

Nice summary, @mbauman.

I'm also curious about the plan in the case we do #24019, then it seems that a user might attempt to use d[1:3] to do non-scalar getindex. Is it correct to say that

  1. We can implement Julep: Generalize indexing with and by Associatives #24019 to always favour scalar indexing when possible (now)
  2. We can Introduce d.[1:3] for non-scalar getindex (in the future)

and that this is all non-breaking for v1.x? Or would we need to do something else? Or go straight for dot-getindex in #24019?

@mbauman
Copy link
Sponsor Member Author

mbauman commented Oct 13, 2017

You've figured out my secret plan: if we figure this out for setindex!, then getindex is really easy and follows naturally.

I'm firmly against allowing non-scalar indexing for dictionaries using the same syntax as scalar getindex. It's simply not useful if you don't know if d[1:2] will give you [d[1],d[2]] or the value at key 1:2. Worse, it would corrupt type inference for, e.g., Dict{Any, Int} — even in the "scalar" case.

The key for #24019 is to figure out if we can define construction, iteration, and conversion in a consistent fashion that sets us up to use dictionaries and arrays more similarly. And then, yes, in 1.x we could aim to define .[] in a manner that dictionaries, arrays, and any other indexable structure could sensibly use it.

@mbauman
Copy link
Sponsor Member Author

mbauman commented May 2, 2018

The work for milestone 1.0 has been addressed in #26347. The remaining tasks are 1.x or 2.0 sorts of things.

@mbauman mbauman removed this from the 1.0 milestone May 2, 2018
@mbauman mbauman removed their assignment May 2, 2018
@StefanKarpinski StefanKarpinski added this to the 1.x milestone May 9, 2018
@DilumAluthge DilumAluthge removed this from the 1.x milestone Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:broadcast Applying a function over a collection kind:breaking This change will break code
Projects
None yet
Development

No branches or pull requests

5 participants