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

In broadcasted assignment, indexing on LHS is a view but indexing on RHS is not #35171

Open
benninkrs opened this issue Mar 19, 2020 · 6 comments
Labels
broadcast Applying a function over a collection

Comments

@benninkrs
Copy link

benninkrs commented Mar 19, 2020

This is from https://discourse.julialang.org/t/what-is-the-fastest-way-to-update-a-particular-row-of-a-matrix-using-dot-syntax/36160/15.

The issue is that in an expression like x[inds] .+= y, the x[inds] on the LHS of the assignment is broadcasted, but the implied x[inds] on the RHS is not (cf. %2 below):

Meta.@lower x[inds] .+= y
:($(Expr(:thunk, CodeInfo(
    @ none within `top-level scope'
1 ─ %1 = Base.dotview(x, inds)
│   %2 = Base.getindex(x, inds)
│   %3 = Base.broadcasted(+, %2, y)
│   %4 = Base.materialize!(%1, %3)
└──      return %4
)))) 

This is both surprising and performance-degrading as it creates an unnecessary allocation. In contrast, x .+= y is lowered in such a way that all terms on both LHS and RHS are fused into a single broadcast.

It seems like it would be generally desirable to have [...] fuse into broadcasting on both sides of an assignment. However, as discussed in #19169 there are multiple possible semantics for broadcasted indexing, so we would have to think carefully about how the lowering was done so as not to make other behaviors inconsistent.

See also #35158 for a performance issue prompted by the same discourse topic that prompted this issue.

@mbauman mbauman changed the title In broadcasted assignment, indexing on LHS is fused but indexing on RHS is not In broadcasted assignment, indexing on LHS is a view but indexing on RHS is not Mar 19, 2020
@mbauman
Copy link
Sponsor Member

mbauman commented Mar 19, 2020

Yes, I agree this isn't great. The view on the LHS is required by its mutating semantics, whereas the getindex on the RHS is due to the "simple sugar" definition that A[I] .+= x is just A[I] .= A[I] .+ x. I often just recommend using @views any time you find yourself broadcasting with indexing.

(I've updated the title to clarify that it's a view, not a fuse.)

@mbauman mbauman added the broadcast Applying a function over a collection label Mar 19, 2020
@benninkrs
Copy link
Author

Somehow I had assumed that in broadcasting expressions, indexing automatically created a view. Now that I think more about it, I rather like the idea of using the syntax x.[i] to opt-in to broadcast indexing, as @StefanKarpinski suggested in #19169.

As an aside, I think there is a close connection between views and fusing, The manual says:

nested "dot calls" are fusing: they are combined at the syntax level into a single loop, without allocating temporary arrays.

My thinking is that in an expression like x[inds] .= y , the view of x is effectively nesting a broadcast of getindex over inds, in a way that avoids allocating a temporary array, within a broadcast of = over the LHS and RHS. So in that sense I was thinking of the view as being "fused" into the broadcast assignment.

@mbauman
Copy link
Sponsor Member

mbauman commented Mar 20, 2020

If you've not seen it, there's the much more comprehensive #30845. I agree the distinction between views and fuses is pedantic, but there's more overhead in the former and there are bigger implications in how we fuse into dot-expressions on the inside of the indexing and the APL semantics.

@benninkrs
Copy link
Author

Thanks,. I think I saw that discussion at some point but had since forgotten about it.

@tkf
Copy link
Member

tkf commented Mar 21, 2020

I think it's still nice to make indexing lazy automatically. In principle we can fuse filtering and mapping with x[predicate.(x)] .+= 1. It is not adequate to use x.[i] syntax for boolean indexing like this. However, to make this efficient, we need x[idx] to have non-copying semantics within broadcasting.

I think it is helpful to consider the issue here as lazy view rather than fusing indexing with broadcasting. Is there a problem to lower f.(x[i]) to

a = Base.dotviewr(x, i)  # like dotview but for RHS
b = Base.broadcasted(f, a)
c = Base.materialize(b)

?

@AshtonSBradley
Copy link

so how would I do something like

@. x[x>1]+=sign(x)*x

which feels like a natural thing to want to do with broadcasting, but gives errors about object sizes. Seems to be that the RHS cannot depend on x (i.e. some scalar value RHS runs). Is there some workaround that lets some of the @. survive?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection
Projects
None yet
Development

No branches or pull requests

4 participants