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

Behavior of BroadcastFunction is inconsistent for ranges #40309

Closed
jishnub opened this issue Apr 2, 2021 · 3 comments · Fixed by #40330
Closed

Behavior of BroadcastFunction is inconsistent for ranges #40309

jishnub opened this issue Apr 2, 2021 · 3 comments · Fixed by #40330
Labels
broadcast Applying a function over a collection

Comments

@jishnub
Copy link
Contributor

jishnub commented Apr 2, 2021

The docstring for Base.BroadcastFunction says that it

Represents the "dotted" version of an operator, which broadcasts the operator over its arguments, so BroadcastFunction(op) is functionally equivalent to (x...) -> (op).(x...).

Except for ranges

julia> f = +
+ (generic function with 190 methods)

julia> Broadcast.BroadcastFunction(f)(2:3, 2:3)
2-element Vector{Int64}:
 4
 6

julia> f.(2:3, 2:3)
4:2:6

The values are still equal, as we may check with

julia> Base.BroadcastFunction(+)(2:3, 2:3) == (2:3) .+ (2:3)
true

however the types are different. The BroadcastFunction(+) version also allocates whereas the .+ version doesn't.

@simeonschaub
Copy link
Member

Ah, the problem is that Base.broadcasted_kwsyntax replaces + with a closure, so because ranges only intercept Base.broadcasted(+, ...), .+(...; kwargs...), which is what BroadcastFunction actually calls, doesn't get intercepted:

@inline broadcasted_kwsyntax(f, args...; kwargs...) = broadcasted((args...)->f(args...; kwargs...), args...)

We could just special-case isempty(kwargs) here, which would be a simple enough fix. Alternatively, we can also just remove support for kwargs from BroadcastFunction.

@simeonschaub simeonschaub added the broadcast Applying a function over a collection label Apr 2, 2021
@jishnub
Copy link
Contributor Author

jishnub commented Apr 2, 2021

Perhaps removing support for kwargs would be the right move, as there won't be such surprises with other operators as well. One may construct the closure and pass it to BroadcastFunction if the parent function needs kwargs

@simeonschaub
Copy link
Member

I mean, with kwargs, .+(a, b; c=3) would be consistent with BroadcastFunction again.

simeonschaub added a commit that referenced this issue Apr 3, 2021
mcabbott added a commit to mcabbott/julia that referenced this issue Apr 30, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this issue May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this issue May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this issue Jul 5, 2021
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 a pull request may close this issue.

2 participants