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

New .-broadcast throws no method error for promote_eltype_op #18176

Closed
mauro3 opened this issue Aug 22, 2016 · 16 comments
Closed

New .-broadcast throws no method error for promote_eltype_op #18176

mauro3 opened this issue Aug 22, 2016 · 16 comments
Labels
broadcast Applying a function over a collection bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@mauro3
Copy link
Contributor

mauro3 commented Aug 22, 2016

For some reason the last call with two floats throws an error:

# Version 0.5.0-rc2+0 (2016-08-12 11:25 UTC)

julia> f(a,b,c) = a+b+c
f (generic function with 1 method)

julia> f.(1.0:2, 3, 4)
2-element Array{Float64,1}:
 8.0
 9.0

julia> f.(1.0:2, 3.0, 4)
2-element Array{Float64,1}:
 8.0
 9.0

julia> f.(1.0:2, 3.0, 4.0)
ERROR: MethodError: no method matching promote_eltype_op(::#f, ::Float64, ::Float64)
Closest candidates are:
  promote_eltype_op(::Any, ::Any, ::Any, ::Any, ::Any...) at abstractarray.jl:1661
  promote_eltype_op{R,S}(::Any, ::AbstractArray{R,N}, ::S) at abstractarray.jl:1659
  promote_eltype_op{T}(::Any, ::T) at abstractarray.jl:1657
  ...
 in promote_eltype_op(::Function, ::FloatRange{Float64}, ::Float64, ::Float64) at ./abstractarray.jl:1661
 in broadcast(::Function, ::FloatRange{Float64}, ::Float64, ::Float64, ::Vararg{Float64,N}) at ./broadcast.jl:230

note the, according the the manual equivalent, direct broadcast call errors for all arguments:

julia> broadcast(f, 1.0:2, 3, 4)
ERROR: MethodError: no method matching promote_eltype_op(::#f, ::Int64, ::Int64)
Closest candidates are:
  promote_eltype_op(::Any, ::Any, ::Any, ::Any, ::Any...) at abstractarray.jl:1661
  promote_eltype_op{R,S}(::Any, ::AbstractArray{R,N}, ::S) at abstractarray.jl:1659
  promote_eltype_op{T}(::Any, ::T) at abstractarray.jl:1657
  ...
 in promote_eltype_op(::Function, ::FloatRange{Float64}, ::Int64, ::Int64) at ./abstractarray.jl:1661
 in broadcast(::Function, ::FloatRange{Float64}, ::Int64, ::Int64, ::Vararg{Int64,N}) at ./broadcast.jl:230

If this is indeed a bug, maybe backport it to 0.5.

@mauro3
Copy link
Contributor Author

mauro3 commented Aug 22, 2016

Maybe of interest:

julia> expand(:(f.(1.0:2, 3, 4)))
:($(Expr(:thunk, Toplevel LambdaInfo thunk)))

julia> expand(:(f.(1.0:2, 3.0, 4.0)))
:((Base.broadcast)(f,colon(1.0,2),3.0,4.0))

(what does the first expansion actually mean?)

@stevengj
Copy link
Member

As an optimization, the f.(args...) transformation inlines things known to be scalars, and in particular it inlines numeric literals. So, f.(1.0:2, 3.0, 4.0) is supposed to be transformed to broadcast(a -> f(a,3.0,4.0), 1.0:2).

Apparently this is happening with f.(1.0:2, 3, 4), but not with the floating-point case? I'm not sure why. Running (fe "f.(1.0:2, 3.0, 4.0)") in julia --lisp seems to indicate that it is inlined in the same way as for integers.

@stevengj stevengj added the broadcast Applying a function over a collection label Aug 22, 2016
@stevengj
Copy link
Member

(@pabloferz, does #16986 fix the error for broadcast(f, 1.0:2, 3, 4)? We obviously want to handle this in any case.)

@pabloferz
Copy link
Contributor

pabloferz commented Aug 22, 2016

@stevengj Yes, that example and all of the OP ones should work fine over #16986

#=
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.6.0-dev.311 (2016-08-19 09:33 UTC)
 _/ |\__'_|_|_|\__'_|  |  pz/broadcast/8877bcb (fork: 3 commits, 3 days)
|__/                   |  x86_64-linux-gnu
=#

julia> f(a,b,c) = a + b + c
f (generic function with 1 method)

julia> f.(1.0:2, 3, 4)
2-element Array{Float64,1}:
 8.0
 9.0

julia> f.(1.0:2, 3.0, 4.0)
2-element Array{Float64,1}:
 8.0
 9.0

julia> broadcast(f, 1.0:2, 3, 4)
2-element Array{Float64,1}:
 8.0
 9.0

@pabloferz
Copy link
Contributor

pabloferz commented Aug 22, 2016

Of course, the parsing problem should be handled separately to get the fusing optimization.

@stevengj
Copy link
Member

stevengj commented Aug 22, 2016

Okay, the problem with the fusing is that 3.0 is not treated as a number in Scheme, it is a "Julia value" (i.e. a pointer to a Julia object, rather than a scheme number object), whereas integer literals are apparently treated as Scheme number objects. (I guess this is because Scheme doesn't have enough numeric types to reflect all of Julia's literals. It doesn't happen in julia --lisp mode, because the Julia language is not available then... when scheme is run via Julia, ast.c has a function scm_to_julia_ that does this conversion.)

@stevengj
Copy link
Member

stevengj commented Aug 22, 2016

@JeffBezanson, I suppose when compress-fuse in julia-syntax.scm encounters an object with (eq? (typeof e) 'julia_value), it could call back to Julia somehow (similar to how jl_static_print works) in order to query the broadcast.jl code to ask if the object is a scalar?

Or we could just drop this optimization, but I was really hoping expressions like 3x.^2 .+ 5x .- 17x.^3 could turn into broadcast(x -> 3x^2 + 5x - 17x^3, x) rather than broadcast((x, _1, _2, _3, _4, _5) -> _1 * x^_2 + _3*x - _4*x^_5, x, 3, 2, 5, 17, 3).

@TotalVerb
Copy link
Contributor

Alternatively, one might consider parsing x.(a, b, c) as @broadcast x a b c and then doing the "lowering" in Julia.

@stevengj
Copy link
Member

@pabloferz, would it be possible to construct a minimal fix for this particular broadcast problem without pulling in the new functionality of #16986 (which is probably too late to go into 0.5)?

@pabloferz
Copy link
Contributor

pabloferz commented Aug 23, 2016

Yeah, I believe so, I'll submit a PR. Should I also add the easy case of broadcasting over tuples of the same size (that is just one or two lines)?

@stevengj
Copy link
Member

stevengj commented Aug 23, 2016

@pabloferz, no that would violate the 0.5 feature freeze. Just the fix for broadcasting over mixtures of arrays and scalars would be great.

@stevengj
Copy link
Member

stevengj commented Aug 23, 2016

(@TotalVerb, changing this to a macro is tricky too. Putting aside the effort of rewriting all the fusion code, there is the issue that macro expansion happens before things like a .+= b are converted to a .= a .+ b, which is too early, so you would have to move more and more of julia-syntax.scm into Julia. However, I think there should be a simpler fix to tag literals for the parser that doesn't require a Julia callback. See #18202.)

pabloferz added a commit to pabloferz/julia that referenced this issue Aug 23, 2016
@StefanKarpinski StefanKarpinski added this to the 0.5.x milestone Aug 23, 2016
@StefanKarpinski StefanKarpinski added the bug Indicates an unexpected problem or unintended behavior label Aug 23, 2016
pabloferz added a commit to pabloferz/julia that referenced this issue Aug 24, 2016
@ninegua
Copy link

ninegua commented Aug 25, 2016

Maybe not exactly related to the bug reported here, but at least the error message is the same:

julia> broadcast(*, [1,1,1], 2, [2,3,4])
3-element Array{Int64,1}:
 4
 6
 8

julia> broadcast(*, [1,1,1], [2,3,4], 2)
ERROR: MethodError: no method matching promote_eltype_op(::Base.#*, ::Type{Int64}, ::Int64)
Closest candidates are:
  promote_eltype_op(::Any, ::Any, ::Any, ::Any, ::Any...) at abstractarray.jl:1665
  promote_eltype_op{T}(::Any, ::AbstractArray{T,N}, ::Any) at abstractarray.jl:1662
  promote_eltype_op(::Any, ::Any) at abstractarray.jl:1660
  ...
 in promote_eltype_op(::Function, ::Array{Int64,1}, ::Array{Int64,1}, ::Int64) at ./abstractarray.jl:1665
 in broadcast(::Function, ::Array{Int64,1}, ::Array{Int64,1}, ::Int64, ::Vararg{Int64,N}) at ./broadcast.jl:230

This is with latest release-0.5 branch. Believe this is a recently introduced issue, as the code worked fine a few weeks ago.

@stevengj
Copy link
Member

I think this is fixed by #18200?

@pabloferz
Copy link
Contributor

Yes, that should fix this too.

@ninegua
Copy link

ninegua commented Aug 25, 2016

Good to know. Thanks!

pabloferz added a commit to pabloferz/julia that referenced this issue Aug 26, 2016
tkelman pushed a commit that referenced this issue Aug 29, 2016
* Fix #18176 (broadcast over mixtures of arrays and numeric scalars)

* Fix #17984 (broadcast behavior over empty arrays)

(cherry picked from commit a232a0b)
tkelman pushed a commit that referenced this issue Aug 29, 2016
* Fix #18176 (broadcast over mixtures of arrays and numeric scalars)

* Fix #17984 (broadcast behavior over empty arrays)

(cherry picked from commit a232a0b)
mfasi pushed a commit to mfasi/julia that referenced this issue Sep 5, 2016
* Fix JuliaLang#18176 (broadcast over mixtures of arrays and numeric scalars)

* Fix JuliaLang#17984 (broadcast behavior over empty arrays)
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 bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

6 participants