Fix for setindex! with array values

[torfjelde]

The current possible implementation for setindex! only allows scalars as values, and so something like x[:, 1] = zeros(size(x, 1)) will result in a Matrix{Any}.

This of course has significant perf. implications, so it seems worth it to specialize on these cases.

Related: JuliaFolds/BangBang.jl#238 and TuringLang/DynamicPPL.jl#530

[torfjelde]

Possibly stupid idea: could we just do >= M instead of == M?

[MasonProtter]

I don't have a good intuition for that. I guess the question would be: can we cook up an example that would be incorrect / error if we did >=?

[torfjelde]

I'd also wager that we could use some functionality from Base for all this reasoning about indexes, etc.?

[torfjelde]

Would love some feedback on this! @MasonProtter maybe?:)
In Turing.jl this has quite detrimental performance implications, so it would be nice to get sorted.

[yebai]

cc @StefanKarpinski

[MasonProtter]

Hey @torfjelde I'm really sorry for the slow reply. This looks like it's on the right track, but I think there's still some cases that need to be caught, like I think this won't work with

setindex!([1,2,3,4,5,6], [7,8,9], 1:3)

right?

Maybe in possible, a better thing to do would be to test if

size(view(collection, indices...)) == size(value)

? Do you foresee any problems with that? Or maybe this isn't even possible's job, and we should instead throw an error inside the setindex!! impl if they give indices that don't make sense with the size of the value being inserted.

[MasonProtter]

Why the restrictions to <:Real here?

[torfjelde]

I don't remember tbh 😕 Might have been me just wanting to limit the extent of this change to avoid potential bugs.

[MasonProtter]

I don't have a good intuition for that. I guess the question would be: can we cook up an example that would be incorrect / error if we did >=?

[MasonProtter]

@torfjelde what can I do to help get this PR moving again? Is it still causing problems with Turing?

[torfjelde]

This looks like it's on the right track, but I think there's still some cases that need to be caught, like I think this won't work with

That example does work as it hits the AbstractVector dimension-calculation (just checked) :)

Maybe in possible, a better thing to do would be to test if

I was under the impression that we wanted to avoid runtime checks in possible, but it might be worth it in this scenario.

what can I do to help get this PR moving again? Is it still causing problems with Turing?

Sorry about the delay Mason; we just "addressed" this by type-piracy for now so kind of filed this PR away. But I think the current version is working, it's just a matter of determining the scope of the change, i.e. should it also apply to non-Real element types?

[MasonProtter]

That example does work as it hits the AbstractVector dimension-calculation (just checked) :)

huh yeah I'm not sure what I was thinking of then, sorry for the noise.

Sorry about the delay Mason; we just "addressed" this by type-piracy for now so kind of filed this PR away. But I think the current version is working, it's just a matter of determining the scope of the change, i.e. should it also apply to non-Real element types?

Not at all a problem, I just wanted to make sure this good work you've done here doesn't get lost. I just checked out the PR again, and tried removing the <:Real and everything seems to work, as well as stuff like this:

With the <:Real restriction:

julia> let v = [(1,),(2,),(3,)]
            setindex!!(v, [(4,), (5,), (6,)], :, 1)
            v
       end
3-element Vector{Tuple{Int64}}:
 (1,)
 (2,)
 (3,)

without the restriction:

julia> let v = [(1,),(2,),(3,)]
            setindex!!(v, [(4,), (5,), (6,)], :, 1)
            v
       end
3-element Vector{Tuple{Int64}}:
 (4,)
 (5,)
 (6,)

So I say let's just completely drop the element type restriction, maybe add the above block as a test case and merge?

[MasonProtter]

test failure is unrelated.

[torfjelde]

Wonderful; thanks @MasonProtter !