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

treat .= as syntactic sugar for broadcast! #17510

Merged
merged 7 commits into from
Jul 21, 2016

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jul 20, 2016

As discussed in #16285, this PR treats x .= ... as syntactic sugar for broadcast!(identity, x, ...), and furthermore fuses the broadcast loop with any nested "dot" calls. e.g. x .= sin.(y) becomes broadcast!(sin, x, y).

The tricky parts were all done in #17300, so this PR seems fairly straightforward.

To do:

  • Tests
  • Documentation
  • For vectors, special case broadcast!(identity, x, scalar) to call fill! and broadcast!(identity, x, y) to call copy!.
  • Fix expression printing for dotted assignments.

This should be a very non-disruptive change, because the .= assignment syntax is new.

Note that x .+= y and so on is now transformed to x .= x .+ y. However, dot operators like x .+ y are not yet treated as dot calls (+).(x,y) (and hence are not yet fused), which will be a more disruptive change.

@StefanKarpinski
Copy link
Member

Good to get these in before 0.5 (might as well take advantage of the otherwise unfortunate delays).

@stevengj
Copy link
Member Author

Should be good to go as soon as I add a NEWS item. (I'd prefer to do that right before merging since NEWS items generate so many conflicts.)

I'm seeing some apparently unrelated Travis and AppVeyor failures on certain platforms, but it appears that master has the same problems.

@stevengj stevengj changed the title WIP: treat .= as syntactic sugar for broadcast! treat .= as syntactic sugar for broadcast! Jul 20, 2016
@stevengj stevengj mentioned this pull request Jul 20, 2016
5 tasks
const expr_infix_wide = Set{Symbol}([
:(=), :(+=), :(-=), :(*=), :(/=), :(\=), :(^=), :(&=), :(|=), :(÷=), :(%=), :(>>>=), :(>>=), :(<<=),
:(.=), :(.+=), :(.-=), :(.*=), :(./=), :(.\=), :(.^=), :(.&=), :(.|=), :(.÷=), :(.%=), :(.>>>=), :(.>>=), :(.<<=),
:(&&), :(||), :(<:), :(=>), :($=)])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does .$= not work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkelman, .$= is not supported by the parser – it was not added by #17393 because the $ is pretty special (e.g. in macros you can do a.$b) and it seemed like it would require more parser hacking to support .$= than it was worth at this point in the 0.5 cycle.

@stevengj
Copy link
Member Author

@StefanKarpinski, @tkelman, tests are green. Okay to merge once I add a NEWS item?

@StefanKarpinski
Copy link
Member

Ok by me. May warrant a @JeffBezanson review due to flisp slinging.

@stevengj
Copy link
Member Author

The flisp changes are pretty trivial (it only looks like a lot of lines because I did a little refactoring of #17300, but most of those lines are just indentation changes and similar).

@stevengj
Copy link
Member Author

stevengj commented Jul 21, 2016

Uh oh, I just realized a problem with this PR for backward compatibility.

If you have an expression like:

x[:] .*= 2

then it already updates x in-place. However, that now turns into broadcast!(x -> x .* 2, x[:], x[:]), and because x[:] makes a copy (#13157) this actually does not update x.

I think I need to update the PR so that it detects when the lhs is a getindex expression and calls view to get a view.

@stevengj
Copy link
Member Author

Okay, I have a PR almost ready to fix the x[...] .= ... problems. Fortunately, the fix was easy.

stevengj added a commit to stevengj/julia that referenced this pull request Jul 21, 2016
stevengj added a commit that referenced this pull request Jul 22, 2016
* make x[...] .= ... assign in-place (fixes bug in #17510)

* doc fix

* You're the top! You're the Coliseum.

* better var name
stevengj added a commit that referenced this pull request Jul 22, 2016
@stevengj stevengj added the broadcast Applying a function over a collection label Aug 2, 2016
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
* treat .= as syntactic sugar for broadcast!

* tests

* optimized .= assignment of scalars and vector copies

* .= documentation

* fix show of .= ops

* .-= tests

* NEWS for .=
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
* make x[...] .= ... assign in-place (fixes bug in JuliaLang#17510)

* doc fix

* You're the top! You're the Coliseum.

* better var name
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
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

Successfully merging this pull request may close these issues.

3 participants