Fix indexing

[willtebbutt]

See here

[MikeInnes]

Can you summarise the issue/fix? I guess this should be AbstractArray; it seems like you'd want to handle the general case where a mix of arrays and integers can be used, in which case perhaps we need to only special case i::Integer...?

[willtebbutt]

The issue is that there was a bug introduced into broadcast!: see here for discussion and here for the fix. It meant that if you write

x = [0]
x[[1, 1]] .+= 1

then x[1] == 1 not 2. If you take a look at the lowered code it's clear what's going on mechanically:

julia> Meta.@lower x[[1, 1]] .+= 1
:($(Expr(:thunk, CodeInfo(
1 ─ %1 = (Base.vect)(1, 1)
│   %2 = (Base.dotview)(x, %1)
│   %3 = (Base.getindex)(x, %1)
│   %4 = (Base.broadcasted)(+, %3, 1)
│   %5 = (Base.materialize!)(%2, %4)
└──      return %5
))))

The upshot is that it causes incorrect gradients with getindex if you have repeated indices. For example

gradient(x->sum(x[[1, 1]]), randn())

yields 1 rather than 2.

The @views makes %3 a view rather than a copy of x:

julia> Meta.@lower @views x[[1, 1]] .+= 1
:($(Expr(:thunk, CodeInfo(
1 ─      ##a#363 = x
│        i#364 = (Base.vect)(1, 1)
│   %3 = (Base.dotview)(##a#363, i#364)
│   %4 = (Base.maybeview)(##a#363, i#364)
│   %5 = (Base.broadcasted)(+, %4, 1)
│   %6 = (Base.materialize!)(%3, %5)
└──      return %6
))))

which fixes this issue.

I agree that we should generalise the indices to include more than just AbstractArrays of indices. I didn't originally restrict the type of the indices, but wound up with some bugs if I did. Any idea why that would be a thing @MikeInnes (sorry for the very general question...)

[MikeInnes]

I see, thanks for the explanation. It's unfortunate that @views x[1] .+= 1 doesn't work (don't know if that causes the error you were seeing). It seems to me like we need the current definition for Integer... and the new one for everything else.

Am I correct in understanding that this fix will be unnecessary on newer julia versions? If so might be worth a comment to that effect.

[willtebbutt]

It's unfortunate that @views x[1] .+= 1 doesn't work

Eurgh, good point. This is all a bit confusing.

Am I correct in understanding that this fix will be unnecessary on newer julia versions?

You are. Do you know if there's 1.1.x release planned? If so I imagine this issue would be fixed on that version.

[MikeInnes]

No idea; even with a release upcoming it does seem like a good idea to have the workaround available though.