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

broadcasting over scalars should produce a scalar #17318

Merged
merged 2 commits into from
Jul 11, 2016

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jul 7, 2016

This changes f.(numbers...) to be equivalent to f(numbers...). Rationale:

  • It is a longstanding convention that "dot" operators like .^ are equivalent to the "non-dot" operator when acting on scalars, and we need to preserve this behavior if we want to change dot operators to use broadcast as per Vectorization Roadmap #16285.
  • It makes it easier to write generic code that works equally well for vectors and scalars.
  • map(f, numbers...) already returned f(numbers...), and it is odd for broadcast to be different.

Also, I made the empty-argument case f.() or map(f) return f(), for much the same reasons. (Previously, this threw an exception for both map and broadcast.) Update: This also will give more consistent behavior for inlining literals in f.(args...) broadcast syntax (see below).

@stevengj
Copy link
Member Author

stevengj commented Jul 7, 2016

cc: @nalimilan

@nalimilan
Copy link
Member

+1, but see #17053.

@stevengj
Copy link
Member Author

stevengj commented Jul 7, 2016

While it would be nice to have a more general mechanism, I'd prefer to merge this so that the f.(number...) semantics are fixed for 0.5.

@nalimilan
Copy link
Member

Sure, let's merge and possibly revise this to use the new trait from #17053 later.

Another interesting question: should strings behave like scalars? That's been advocated at #16966 (comment). Again, should not block this PR.

@@ -197,6 +197,10 @@ let a = broadcast(Float32, [3, 4, 5])
@test eltype(a) == Float32
end

# broadcasting scalars:
@test sin.(1) == broadcast(sin, 1) == sin(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

test type equality too?

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

@stevengj
Copy link
Member Author

stevengj commented Jul 7, 2016

At least for the purposes of broadcast, my inclination is that strings should be treated as scalars. I'd like to be able to do broadcast(string, "foo", 1:3) and get ["foo1", "foo2", "foo3"]. But that is a separate issue from this PR, of course.

@TotalVerb
Copy link
Contributor

TotalVerb commented Jul 8, 2016

Why should it return f() (in analogy with map(f, 1, 2)), and not [f()] (in analogy with map(f, [1], [2])), or even [f(), f()] in analogy with map(f, [1, 2], [3, 4])? It seems it's impossible for the empty case to be fully generic, because different kinds of collections give different results.

Otherwise, I agree with this PR.

@pabloferz
Copy link
Contributor

I was doing this also as part of #16986. But that tries to be much more general. I agree that at least the simple case for number should work on 0.5.

@andyferris
Copy link
Member

Why should it return f() (in analogy with map(f, 1, 2)), and not [f()](in analogy with map%28f, [1], [2]%29), or even [f(), f()] in analogy with map(f, [1, 2], [3, 4])?

I think it would do all of those things. broadcast(f, 1, 2) == map(f, 1, 2) == f(1,2), and broadcast(f, [1], [2]) == map(f, [1], [2]) == [f(1,2)], etc, no? I.e. [1] is not a scalar.

@stevengj
Copy link
Member Author

stevengj commented Jul 8, 2016

@TotalVerb, my feeling was that f.() should return the simplest possible sensible result. The result should certainly be zero-dimensional for broadcast over zero arrays, and f() is a much simpler and more useful "zero-dimensional" result than a zero-dimensional Array. And map should be consistent with broadcast.

@stevengj
Copy link
Member Author

stevengj commented Jul 9, 2016

Another reason for this f.() behavior: in my loop-fusion code for #17300, pretty soon I will be "inlining" numeric literals, so that atan2.(x,1) goes to broadcast(x -> atan2(x,1), x). However, in that case it will transform atan2.(3,4) to broadcast(() -> atan2(3,4)), and the f.() === f() behavior is required for this to work.

(I could explicitly check for the case where all the arguments are literals, of course, but the inlining behavior is much more consistent if I don't have to special-case this.)

@stevengj
Copy link
Member Author

stevengj commented Jul 9, 2016

Okay to merge? cc @JeffBezanson

@stevengj stevengj added the needs decision A decision on this change is needed label Jul 11, 2016
@stevengj stevengj added this to the 0.5.0 milestone Jul 11, 2016
@stevengj
Copy link
Member Author

These semantics really should be decided by 0.5, because the broadcast usage will expand greatly with the new f.(args...) syntax, so I'm marking this with the milestone.

stevengj added a commit to stevengj/julia that referenced this pull request Jul 11, 2016
@JeffBezanson JeffBezanson merged commit 0822970 into JuliaLang:master Jul 11, 2016
stevengj added a commit that referenced this pull request Jul 11, 2016
@stevengj stevengj deleted the bcast_scalar branch July 11, 2016 23:53
stevengj added a commit that referenced this pull request Jul 12, 2016
# fallback routines for broadcasting with no arguments or with scalars
# to just produce a scalar result:
broadcast(f) = f()
broadcast(f, x::Number...) = f(x...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is ambiguous with

function broadcast(x::Any, i::Union{Integer,Symbol})
(but not detected? cc @timholy) and causes, for example speye(5).(2) to not trigger the deprecation getfield path that it would have meant on 0.4, but instead trigger a MethodError: objects of type SparseMatrixCSC{Float64,Int32} are not callable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be a simple matter of adding a more specific deprecation method, yes?

Copy link
Contributor

Choose a reason for hiding this comment

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

is the deprecation method already more specific? integer vs number? or is the union making dispatch pick Number?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, the deprecation method seems more specific according to the rules discussed in #16489, but maybe the varargs are confusing things here. @JeffBezanson?

Copy link
Member Author

@stevengj stevengj Aug 3, 2016

Choose a reason for hiding this comment

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

Oh, right, it's turning foo.(1) into broadcast(() -> foo(1)) because the literal gets inlined into the function by compress-fuse, so the deprecation can't be invoked.

Copy link
Member Author

@stevengj stevengj Aug 3, 2016

Choose a reason for hiding this comment

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

Will submit a PR shortly to restore the getfield deprecation.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, I was attempting some things and not able to get it to work - d8f4b60 wasn't where I thought bisect would point me

mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants