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

Discrepancy b/w map!(f, A) and broadcast!(f, A) #12277

Open
davidagold opened this issue Jul 23, 2015 · 22 comments
Open

Discrepancy b/w map!(f, A) and broadcast!(f, A) #12277

davidagold opened this issue Jul 23, 2015 · 22 comments
Labels
broadcast Applying a function over a collection

Comments

@davidagold
Copy link
Contributor

Calling map! on a single Array argument applies the appropriate 1-arg method of f on each element of A in-place, whereas calling broadcast! on a single Array argument replaces each element of A with the result of calling the 0-arg method of f:

julia> A = [1:10...];

julia> f(x) = 5x
f (generic function with 1 method)

julia> map!(f, A)
10-element Array{Int64,1}:
  5
 10
 15
 20
 25
 30
 35
 40
 45
 50

julia> broadcast!(f, A)
ERROR: MethodError: `f` has no method matching f()
 in _F_ at broadcast.jl:98
 in broadcast! at broadcast.jl:229

julia> A = Array(Float64, 10);

julia> broadcast!(rand, A)
10-element Array{Float64,1}:
 0.355975
 0.129265
 0.203295
 0.82875 
 0.441758
 0.740748
 0.649689
 0.449172
 0.620475
 0.931164

Not sure if this small point bugs anybody, but I thought I'd call it to attention.

@toivoh
Copy link
Contributor

toivoh commented Jul 23, 2015

Well, that's a tricky case. I'd argue that the broadcast! behavior is more consistent with the methods that take more arguments, but I definitely see the expectation that either function would work as map!(f, A) currently does. It would be good to pick one behavior and apply to both, but it might also be a pretty breaking change.

@davidagold
Copy link
Contributor Author

I agree that the broadcast! behavior is more consistent. Also, whereas a user can simply call broadcast!(f, A, A) to apply the 1-arg method in-place, the only other way (that I can think of, at least) to achieve the broadcast!(f, A) behavior is to write a loop. (Or do something like A = [ f() for a in A ], which seems wasteful.)

There are other discrepancies, too, between map and broadcast, though these have to do with the algorithm map uses to determine the eltype of the resultant container:

julia> A = rand(1:2, 10)
10-element Array{Int64,1}:
 2
 1
 1
 1
 2
 2
 1
 1
 2
 2

julia> f(x) = x > 1 ? 1.5 : 1
f (generic function with 1 method)

julia> map(f, A)
10-element Array{Real,1}:
 1.5
 1  
 1  
 1  
 1.5
 1.5
 1  
 1  
 1.5
 1.5

julia> broadcast(f, A)
ERROR: InexactError()
 in setindex! at array.jl:303
 in _F_ at broadcast.jl:98
 in broadcast! at broadcast.jl:229
 in broadcast at broadcast.jl:236

@timholy
Copy link
Member

timholy commented Jul 26, 2015

Relevant discussion most recently in #12292.

@toivoh
Copy link
Contributor

toivoh commented Jul 27, 2015

But this issue is not about the element type of returned results; it's about whether it's possible to use e.g. map! to map a zero argument call f() over the elements of a destination array. Or am I missing something?

@timholy
Copy link
Member

timholy commented Jul 27, 2015

I was responding to "other discrepancies" part of his post above mine.

@toivoh
Copy link
Contributor

toivoh commented Jul 27, 2015 via email

@stevengj
Copy link
Member

stevengj commented May 5, 2016

See also #4883.

@Sacha0
Copy link
Member

Sacha0 commented Dec 24, 2016

Stumbled upon the map!(f, A) = map!(f, A, A) while broadcast!(f, A) = fill!(A, f()) difference during work on sparse higher order functions. Some thoughts:

The behavioral difference between map!(f, A) and broadcast!(f, A) seems to be choice of implicit source container. Specifically, broadcast!(f, A) = fill!(A, f()) takes something empty and shape-broadcast-compatible with A as the implicit source, whereas map!(f, A) takes A as the implicit source.

On the one hand, as @toivoh and @davidagold note above, broadcast!'s behavior seems consistent with broadcast! methods accepting more arguments. On the other hand, having broadcast!(f, A) instead take A as the implicit source (as in present map!(f, A)) might be more useful: broadcast!(f, A) is a strange way to spell fill!(A, f()), and a broadcast!(f, A) = map!(f, A, A) method without potential aliasing-induced issues might be more useful (though perhaps better spelt map!(f, A)...).

Similarly, on the one hand map!(f, A)'s present behavior (taking A as the implicit source) seems useful as a map!(f, A, A) method without potential aliasing-induced issues. On the other hand, having map!(f, A) instead take something empty as the implicit source might be more consistent with map! methods accepting more arguments, but perhaps less useful (as then the sensible behavior would be map!(f, A) yielding a DimensionMismatch (or similar)).

Should we call this difference a discrepancy and remedy it, or instead simply call it part of the general behavioral difference between map and broadcast and close this issue? Thoughts? Thanks and best!

@stevengj
Copy link
Member

I would prefer to have map behave like broadcast here.

@Sacha0
Copy link
Member

Sacha0 commented Dec 24, 2016

I would prefer to have map behave like broadcast here.

Do you mean have map!(f, A) throw, or map!(f, A) = fill!(A, f())? If the latter, is there a mental model in which that behavior is consistent with map! methods accepting more arguments? Thanks!

@stevengj
Copy link
Member

map!(f, A, args...) stores f(elements of args...) in A. So it is consistent with that (and broadcast!) that map!(f, A) should store f() in A.

(It should not be the same as fill! because f may have side effects. e.g. map!(rand, A) would call rand() separately for each element of A.)

@Sacha0
Copy link
Member

Sacha0 commented Dec 25, 2016

map!(f, A, args...) stores f(elements of args...) in A. So it is consistent with that (and broadcast!) that map!(f, A) should store f() in A.

map!(f, A) occurs only a handful of times in base, so this change should not cause much pain internally. To avoid silent breakage outside base, would you deprecate map!(f, A) for a cycle and then reintroduce map!(f, A) with new semantics, or take some other approach?

Do we generally guarantee that map!/broadcast!'s destination can safely alias one or more of its sources? The sparse map!/broadcast! does not presently provide that guarantee. I suppose we could check whether any(src -> dest === src, srcs) and branch to less efficient code that handles the source-destination aliasing case? Best!

@stevengj
Copy link
Member

Deprecating map!(f, A) -> map!(f, A, A) sounds best. I can find several packages using the two-argument form.

(I notice that FunctionalData.jl actually defines map!(A, f), which seems like an odd choice in Julia; cc @rened.)

@Sacha0
Copy link
Member

Sacha0 commented Dec 25, 2016

Would likewise deprecating broadcast!(f, A) for a cycle be best, or would changing its semantics from fill!(A, f()) to population via independent f() calls be better done silently?

@stevengj
Copy link
Member

I can't imagine that anyone is using broadcast!(f, A) for an f() with side effects right now, since the current semantics make that a very odd thing to do. I would just change it, and make a note under "breaking changes" in the NEWS.

@stevengj
Copy link
Member

stevengj commented Dec 31, 2016

@Sacha0, in 0.6 master, I already get:

julia> broadcast!(rand, zeros(5))
5-element Array{Float64,1}:
 0.103415
 0.856194
 0.278624
 0.714237
 0.643863

whereas in Julia 0.5 I get

julia> broadcast!(rand, zeros(5))
5-element Array{Float64,1}:
 0.969826
 0.969826
 0.969826
 0.969826
 0.969826

so it looks like this was already changed? (If you want fill!(A, rand()) you would do A .= rand() rather than rand.().) There should probably be a NEWS note? Oh, nevermind, I see you changed it in #19722.

@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2016

There should probably be a NEWS note?

Yes there should (also for the various recent deprecations and feature enhancements) but those can wait until after next week when he gets back from some travel.

@Sacha0
Copy link
Member

Sacha0 commented Dec 31, 2016

(Breadcrumb to the completed NEWS item for broadcast!(f, A).)

@mbauman
Copy link
Member

mbauman commented Sep 15, 2017

After #19721, all that remains here is implementing the behavior change such that map! matches broadcast!.

@mbauman mbauman added this to the 1.0 milestone Sep 15, 2017
@JeffBezanson
Copy link
Member

Since map(f, A) is deprecated, what changes exactly are needed for 1.0 at this point?

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Dec 28, 2017
@Sacha0
Copy link
Member

Sacha0 commented Dec 28, 2017

None strictly necessary for 1.0 IIRC. Implementing map!(f, A) matching broadcast!(f, A) for 1.0 might be nice, but could just as well happen in 1.x. Best!

@JeffBezanson
Copy link
Member

Excellent, that's what I thought :)

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Dec 28, 2017
@JeffBezanson JeffBezanson removed this from the 1.0 milestone Dec 28, 2017
@StefanKarpinski StefanKarpinski added this to the 1.x milestone Dec 28, 2017
@mbauman mbauman added broadcast Applying a function over a collection and removed broadcast labels Apr 24, 2018
@DilumAluthge DilumAluthge removed this from the 1.x milestone Mar 13, 2022
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

10 participants