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.

Sign up for GitHub

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

Jump to bottom

Make broadcast!(f, A) populate A via independent f() calls #19722

Merged
merged 1 commit into from
Dec 30, 2016
Merged

Make broadcast!(f, A) populate A via independent f() calls #19722

merged 1 commit into from
Dec 30, 2016

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Dec 25, 2016

broadcast!(f, A) presently yields fill!(A, f()). #12277 discusses making broadcast!(f, A) instead populate A via independent f() calls. This pull request realizes that change. Best!

@Sacha0 Sacha0 added the broadcast Applying a function over a collection label Dec 25, 2016
KristofferC
KristofferC reviewed Dec 25, 2016
View reviewed changes
base/sysimg.jl
@@ -78,7 +78,7 @@ Symbol(x...) = Symbol(string(x...))
# specific array types etc.
# --Here, just define fallback routines for broadcasting with no arguments
broadcast(f) = f()
broadcast!(f, X::AbstractArray) = fill!(X, f())
broadcast!(f, X::AbstractArray) = (for I in eachindex(X); X[I] = f(); end; X)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Is there any merit to using @inbounds here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and done. Thanks!

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 29, 2016

Candidate for 0.6? Tentatively adding to the milestone. Best!

@Sacha0 Sacha0 added this to the 0.6.0 milestone Dec 29, 2016
@tkelman
Copy link
Contributor

tkelman commented Dec 29, 2016

worth testing that this results in independent objects from the output of the function, rather than fill which would share references?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 30, 2016

worth testing that this results in independent objects from the output of the function, rather than fill which would share references?

Good call, and done. Thanks!

@tkelman tkelman merged commit 50a8b96 into JuliaLang:master Dec 30, 2016
@andreasnoack andreasnoack mentioned this pull request Dec 30, 2016
Merged
@Sacha0 Sacha0 deleted the twoargbcbang branch December 30, 2016 19:00
@Sacha0
Copy link
Member Author

Sacha0 commented Dec 30, 2016

Thanks for reviewing / merging!

@stevengj stevengj mentioned this pull request Dec 31, 2016
Open
stevengj added a commit that referenced this pull request Dec 31, 2016
@stevengj
NEWS for #19722
fae8438
@stevengj
Copy link
Member

We also need to change broadcast!(f, X, x::Number...) to do the same thing, rather than fill!(X, f(x...)). (I just ran into this when trying X .= rand.() + 3.)

@stevengj
Copy link
Member

stevengj commented Dec 31, 2016 •
edited
Loading

(It could be argued that this is a bugfix that should be backported to Julia 0.5, since Julia 0.4 used independent f() calls in broadcast! too. The specialized broadcast! methods were introduced in 0.5, and I think were mistakenly intended as merely an optimization rather than as a change in behavior.)

@stevengj stevengj mentioned this pull request Dec 31, 2016
Merged
JeffBezanson pushed a commit that referenced this pull request Dec 31, 2016
@stevengj @JeffBezanson
NEWS for #19722
67343fd
tkelman pushed a commit that referenced this pull request Mar 1, 2017
@Sacha0 @tkelman
Make broadcast!(f, A) populate A via independent f() calls rather tha…
4efacb6 
…n fill!(A, f()). (#19722)

(cherry picked from commit 50a8b96)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KristofferC KristofferC KristofferC left review comments

@tkelman tkelman tkelman approved these changes

@pabloferz pabloferz pabloferz approved these changes

Assignees
No one assigned
Labels
broadcast Applying a function over a collection
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Sacha0 @tkelman @stevengj @KristofferC @pabloferz @StefanKarpinski