RFC: Deprecate implicit scalar broadcasting in setindex!

[mbauman]

The current setindex! function (the A[i] = b syntax) does three very different operations:

• Given an index i that refers to only one location (scalar indexing), A[i] = b modifies A in that location to hold b.
• Given an index I that refers to more than one location (non-scalar indexing), A[I] = b can mean one of two things:
  • If b is an AbstractArray (multi-value), assign the values it contains to those locations I in A. That is, essentially, [A[i] = bᵢ for (i, bᵢ) in zip(I, b)].
  • If b is not an AbstractArray (scalar-value), then broadcast its value to every location selected by I in A.

These two different behaviors in the non-scalar indexing case basically make using this function in a generic way impossible. If you want to always set the value b to many indices of A, you cannot use this syntax because b might happen to be an array in some cases, radically changing the meaning of the expression. You need to manually iterate over the indices, using scalar setindex methods. But we now also have the new broadcast! syntax, A[I] .= B, which uses the usual broadcasting semantics to determine how B should fill into the values of A.

This PR deprecates the implicit broadcasting of scalar values in non-scalar indexing in favor of an explicit .= broadcast syntax, leaving multi-value non-scalar indexing intact. This is the exact opposite of PR #24368.

There are still a few things left to do here, making this still a WIP:

• [-] Make setindex_shape_check more stringent To be done in a separate PR. Or maybe not.
• Examine cases where the deprecation message — as suggested — did not work and try to find less-verbose alternatives than an explicit for loop. Edit: This is much improved now that .= nothing works.
• Re-implement the old setindex! methods as either fill! or broadcast! specializations (or both — having broadcast fall back to fill for Scalar() styles)
• Check performance

[mbauman]

Alright, so I hit a fairly major performance snag that is inherent to either of the suggested replacement APIs (fill!(view(A, I...), value) or A[I...] .= value). BitArray had been specializing setindex!(A::BitArray, value, I::BitArray) to operate chunk-wise, 64 bits at a time. Unfortunately, both fill! and broadcast! require constructing a SubArray to encapsulate the indices, and SubArrays require collecting the indices selected by logical masks in order to maintain O(1) indexing. This means that we have a ~100x perf regression in this specific case.

@oschulz had suggested a fill!(A, value, I...) method, but that feels a little strange to me since no indices means filling the whole array. Is this use-case (and deprecation message) worth exporting a whole new function? fillindices!?

[JeffBezanson]

Maybe we can somehow specialize dotview for BitArrays?

[oschulz]

@oschulz had suggested a fill!(A, value, I...) method, but that feels a little strange to me since no indices means filling the whole array.

I think fill! would come quite naturally, and be a good fit, semantically - as long as there are no problems with dispatch, of course. copyto!, for example, also operates on the whole array, unless additional parameters are given.

[mbauman]

That's a compelling argument for fill!.

I'd love to implement a special MaskedBitArray view — it'd be super fast on vectorized functions. But accessing single elements would be pretty darn slow. It would compound the general iterative slowness of BitArrays with an O(~N) lookup. It's slightly improved because we can do a bunch of popcnts to get to the right chunk, but I think that's still too slow for general purpose use.

[mbauman]

Good news! The new optimizer has fixed my performance issues! Running BaseBenchmarks locally with JuliaCI/BaseBenchmarks.jl#195 applied leaves me with just two regressions (with default tuning settings) at the moment:

julia> pairs = leaves(regs)
2-element Array{Any,1}:
 (Any["linalg", "blas", "axpy!"], TrialJudgement(+43.17% => regression))
 (Any["problem", "laplacian", "laplace_sparse_matvec"], TrialJudgement(+74.37% => regression))

The other issue here is that I have an ambiguity problem with the BitMaskedBitArray. That one is easier to fix on #25377, but I'll try to find an amenable solution here (in isolation), too.

[mbauman]

@nanosoldier runbenchmarks(ALL, vs = ":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[mbauman]

Well rebasing this on top of #26891 solved multiple issues. The performance is looking pretty good (I had been seeing 10x regressions before), and I think it's technically no longer breaking.

That is, the deprecations will use the correct replacement, but we're splitting a dual behavior into two discrete APIs. If you depend upon this dual behavior, then neither of the static replacements will be correct and replacing it will be more challenging… and we cannot know at the time of the depwarn what other call sites might exist.

[mbauman]

I'll merge this tomorrow barring any further comments here.

[oschulz]

Thanks again, @mbauman !

[mbauman]

Travis failures were: Linux x86_64 failed due to some httpbin.org issue; macOS was a LLVM build issue.

Appveyor was the httpbin.org issue and a timeout.

[mschauer]

julia/base/indices.jl

Line 158 in b5d4f3c

setindex_shape_check(X, I...) = nothing # Non-arrays broadcast to all idxs

looks like it should not exist now.