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

broadcast picking incorrect result type #4883

Closed
stevengj opened this issue Nov 21, 2013 · 15 comments · Fixed by #16995
Closed

broadcast picking incorrect result type #4883

stevengj opened this issue Nov 21, 2013 · 15 comments · Fixed by #16995
Assignees
Milestone

Comments

@stevengj
Copy link
Member

The following code fails with an InexactError():

X = [1,2,3]
Y = [4 5]
broadcast(atan2, X, Y)

whereas

[atan2(x,y) for x in X, y in Y ]

(albeit producing an array of type Any), while

atan2([1,2,3],[4,5,6])

produces an array of Float64.

Can we improve the type inference so that all three cases can generate Float64 arrays? Note that this is needed for #4363 (for @vectorize_2arg to use broadcast).

@toivoh
Copy link
Contributor

toivoh commented Nov 21, 2013

There's really no way for broadcast to know that atan2 will produce a float result from int arguments, until some kind of type inference (upper bound inference?) is exposed to Julia code. But if you look in broadcast.jl, there is broadcast_T{T}(op::Function, ::Type{T}, As::StridedArray...), which takes a result type as well, and a companion broadcast_T_function(op::Function) to create a specialized op_broadcasted_T(::Type{T}, As::StridedArray...). So if someone is willing to put in some work to define the mapping of input to output types, this is doable already.

I didn't export broadcast_T because I didn't want to bloat the exports to much, but maybe it should be exported and documented. (Of course, broadcast! already allows to specify the result type through through the type of the destination)

@stevengj
Copy link
Member Author

@toivoh, but how does atan2([1,2,3],[4,5,6]) do it? It looks like it is just an untyped comprehension in @vectorize_2arg, which means that Julia has the logic somewhere to infer the type...

@JeffBezanson
Copy link
Member

Comprehensions contain a special bit of magic. It is arguably the ugliest thing in the language. We will have a more general version of it eventually.

One can also do things like Array(typeof(f(x[1],y[1])), ...) but this isn't perfect either.

@stevengj
Copy link
Member Author

Can't broadcast be written to exploit the comprehension magic, at least in common cases?

@stevengj
Copy link
Member Author

stevengj commented May 5, 2016

Duplicate of #16164

@stevengj
Copy link
Member Author

stevengj commented May 5, 2016

As I commented in #16164, it seems that map is now able to do the right thing, so we need to adapt the map algorithm to broadcast.

@StefanKarpinski
Copy link
Member

I hate to add an issue to the 0.5 milestone, but introducing a new syntax and encouraging its use, while it's this broken seems like a bad idea. See #16896; possibly fixed by #16260?

@timholy
Copy link
Member

timholy commented Jun 13, 2016

#16260 cleans up broadcast (gets rid of the "stash eval-ed functions in dictionaries", aka fake @generated functions), but unfortunately doesn't address the eltype.

@StefanKarpinski
Copy link
Member

Broadcasting, map and comprehensions should all pick the same element type and succeed/fail in the same cases, except that broadcast handles more combinations of argument shapes.

@Sacha0
Copy link
Member

Sacha0 commented Jun 16, 2016

map is now able to do the right thing, so we need to adapt the map algorithm to broadcast.

Ref. peripherally related julia-users thread https://groups.google.com/forum/#!searchin/julia-users/broadcast$20map/julia-users/IAZSOCZg1ww/TOvwOZBEAwAJ

@pabloferz
Copy link
Contributor

pabloferz commented Jun 16, 2016

AFAICS, the first example works now on master.

X = [1,2,3]
Y = [4 5]

julia> broadcast(atan2, X, Y)
3×2 Array{Float64,2}:
 0.244979  0.197396
 0.463648  0.380506
 0.643501  0.54042

@JeffBezanson
Copy link
Member

Ah, that appears to be due to which promote_op definitions exist.

@pabloferz
Copy link
Contributor

Exactly, I'm trying to complete and organize them on #16986.

JeffBezanson added a commit that referenced this issue Jun 29, 2016
fix #4883, result type of `broadcast` for arbitrary functions
@stevengj
Copy link
Member Author

stevengj commented Jun 29, 2016

broadcast(sin, [1,2,3]) still fails with an InexactError (because it erroneously thinks the output is an Int array), whereas map(sin, [1,2,3]) succeeds.

@stevengj
Copy link
Member Author

(Other good test cases are broadcast(length, ["foo", "bar"]) or broadcast(length, [[1,2,3], [5]]). We need to make sure that the solution is as general as in map, not restricted to numeric types.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants