Fix length calculation of broadcast over tuples #23887

pabloferz · 2017-09-27T00:25:06Z

Sacha0 · 2017-09-27T01:26:47Z

Was #23647 introduced by #21331? The length determination logic that existed prior to #21331 seems a bit simpler than that in this pull request?

Sacha0 · 2017-09-27T01:31:59Z

base/broadcast.jl

@@ -127,6 +127,7 @@ end
 end

 Base.@propagate_inbounds _broadcast_getindex(A, I) = _broadcast_getindex(containertype(A), A, I)
+Base.@propagate_inbounds _broadcast_getindex(::Type{Tuple}, A::Tuple{Any}, I) = A[1]


Should A[1] not be A[I]?

No, you can see that there is a ::Tuple{Any} in the signature, we need this for handling cases like (1,2,3) .+ (1,). This shouldn't be a problem as we want (x,) where x is a scalar to behave the same as [x] or x.

Yes, I missed the Any last night :). To make this method's purpose clearer, perhaps add a comment or an adjacent general method

Base.@propagate_inbounds _broadcast_getindex(::Type{Tuple}, A::Tuple, I) = A[I]

though it would be redundant with

Base.@propagate_inbounds _broadcast_getindex(::Any, A, I) = A[I]

below?

I have added a comment.

pabloferz · 2017-09-27T04:57:55Z

The length determination logic that existed prior to #21331 seems a bit simpler than that in this pull request?

The problem with the previous approach is that it was hard to get a type stable result, that was the reason for #21331 but that solution wasn't good enough.

Sacha0 · 2017-09-27T17:46:29Z

The problem with the previous approach is that it was hard to get a type stable result, that was the reason for #21331 but that solution wasn't good enough.

Yes, rather I was wondering whether the simpler former logic could be improved while remaining simpler than that in this pull request. A few comments describing the logic in this pull request would do the trick as well :). Best!

pabloferz · 2017-09-28T20:54:19Z

Yes, rather I was wondering whether the simpler former logic could be improved while remaining simpler than that in this pull request.

I couldn't come up with a simpler approach, but I have added comments as you suggested. The wording could probably be improved, so if you have any suggestions about it, I'm all ears.

Sacha0 · 2017-09-29T01:24:04Z

base/broadcast.jl

+# When the result of broadcast is a tuple it can only come from mixing n-tuples
+# of the same length with scalars and 1-tuples. So, in order to have a
+# type-stable broadcast we need to find a tuple of maximum length (except when
+# there are only scalars empty tuples and empty tuples, in which case the


One too many "empty tuples?"

Sacha0 · 2017-09-29T01:26:13Z

💯 for the comments! :)

pabloferz · 2017-09-29T13:45:54Z

Because of a possible mess with ambiguities, I decided to use the containertype trait to disambiguate methods. I believe this also makes the implementation much clearer (in addition to the presence of some comments)

pabloferz · 2017-10-02T20:28:30Z

Has anyone have more comments/objections? If not I'd like to merge this in a day or two.

TotalVerb · 2017-10-02T20:56:09Z

base/broadcast.jl

+    _tuplebroadcast_maxtuple(containertype(A), containertype(B), A, B)
+@inline tuplebroadcast_maxtuple(A, Bs...) =
+    tuplebroadcast_maxtuple(A, tuplebroadcast_maxtuple(Bs...))
+tuplebroadcast_maxtuple(A::NTuple{N,Any}, ::NTuple{N,Any}...) where {N} = A


I think this needs to be a _tuplebroadcast_maxtuple method, for example with the testcase broadcast(+, (1, 2), (3, 4), 5).

That example works with this PR as is. I had it like this to reduce the compiler/inference work in cases where you have a bunch of tuples of the same length and a scalar element (which I imagined would be common), e.g. (1,2) .+ (2,3) .+ (3,4) .+ (4,5) .+ 1. But I can make it a _tuplebroadcast_maxtuple for consistency if preferred.

Sorry, I screwed up the order of my example. It should be broadcast(+, 5, (1, 2), (3, 4)) which I believe would not currently work.

I can confirm that example (or broadcast(+, (1, 2), 5, (3, 4))) works fine.

tuplebroadcast_maxtuple will call _tuplebroadcast_maxtuple only when the number of arguments to tuplebroadcast_maxtuple is two and both are not tuples of the same length (if they are, dispatch would select tuplebroadcast_maxtuple(A::NTuple{N,Any}, ::NTuple{N,Any}...) where {N} instead).

Oh, I understand. Thanks for the correction.

TotalVerb · 2017-10-03T01:24:25Z

Wouldn't a method for two tuples of the same length be necessary anyway? This method can be kept as a specialization, right?

timholy · 2017-10-03T01:33:31Z

In case it's of interest, #23939 has a longest method.

pabloferz · 2017-10-03T18:51:46Z

Wouldn't a method for two tuples of the same length be necessary anyway? This method can be kept as a specialization, right?

Yes a method for tuples of the same length is necessary. And the current one could be changed into a _tuplebroadcast_maxtuple specialization, but I wrote it like this as an optimization for inference/compilation (as opposed to a runtime optimization). This could be not all that important (someone like @vtjnash could check if it's worth it), in which case I can change it.

In case it's of interest, #23939 has a longest method.

Thanks Tim, I don't think both behave the same (but that PR is really nice, I'm going to take a closer look into it).

Sacha0 · 2017-10-03T22:28:23Z

@nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2017-10-04T02:04:04Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Sacha0 · 2017-10-04T02:56:10Z

@nanosoldier runbenchmarks("tuple" || "scalar", vs = ":master")

nanosoldier · 2017-10-04T04:49:10Z

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan

ararslan · 2017-11-07T04:20:53Z

Good to go?

TotalVerb · 2017-11-07T04:26:53Z

@pabloferz Please confirm the behavior of broadcast(+, 5, (1, 2), (3, 4)); while I have not tested the code I still believe the mentioned _tuplebroadcast_maxtuple method to be necessary.

pabloferz · 2017-11-07T14:33:56Z

@TotalVerb the behaviour of broadcast for the examples you have mentioned seems to be the expected with this PR.

(cherry picked from commit 698ef27)

pabloferz added the backport pending 0.6 label Sep 27, 2017

ararslan added collections Data structures holding multiple items, e.g. sets broadcast Applying a function over a collection labels Sep 27, 2017

ararslan requested a review from mbauman September 27, 2017 00:41

Sacha0 reviewed Sep 27, 2017

View reviewed changes

pabloferz force-pushed the pz/tuplebroadcast branch from 7890971 to 332dbaa Compare September 28, 2017 20:29

pabloferz force-pushed the pz/tuplebroadcast branch from 332dbaa to 893dfa2 Compare September 28, 2017 22:36

Sacha0 reviewed Sep 29, 2017

View reviewed changes

Fix length calculation of broadcast over tuples

8049df1

pabloferz force-pushed the pz/tuplebroadcast branch from 893dfa2 to 8049df1 Compare September 29, 2017 03:34

mbauman approved these changes Oct 2, 2017

View reviewed changes

TotalVerb reviewed Oct 2, 2017

View reviewed changes

ararslan merged commit 698ef27 into JuliaLang:master Nov 7, 2017

pabloferz deleted the pz/tuplebroadcast branch November 7, 2017 20:49

ararslan pushed a commit that referenced this pull request Nov 7, 2017

Fix length calculation of broadcast over tuples (#23887)

877ae03

(cherry picked from commit 698ef27)

ararslan pushed a commit that referenced this pull request Nov 8, 2017

Fix length calculation of broadcast over tuples (#23887)

82cd0c9

(cherry picked from commit 698ef27)

ararslan pushed a commit that referenced this pull request Nov 8, 2017

Fix length calculation of broadcast over tuples (#23887)

330e4d9

(cherry picked from commit 698ef27)

ararslan pushed a commit that referenced this pull request Nov 8, 2017

Fix length calculation of broadcast over tuples (#23887)

2048c86

(cherry picked from commit 698ef27)

ararslan pushed a commit that referenced this pull request Nov 8, 2017

Fix length calculation of broadcast over tuples (#23887)

079fe29

(cherry picked from commit 698ef27)

ararslan pushed a commit that referenced this pull request Nov 9, 2017

Fix length calculation of broadcast over tuples (#23887)

da92af7

(cherry picked from commit 698ef27)

ararslan pushed a commit that referenced this pull request Nov 14, 2017

Fix length calculation of broadcast over tuples (#23887)

a2a45bb

(cherry picked from commit 698ef27)

ararslan mentioned this pull request Nov 19, 2017

WIP: Backports for 0.6.2 #24519

Merged

ararslan pushed a commit that referenced this pull request Nov 21, 2017

Fix length calculation of broadcast over tuples (#23887)

4ba27b9

(cherry picked from commit 698ef27)

ararslan removed the backport pending 0.6 label Nov 29, 2017

fredrikekre mentioned this pull request May 23, 2018

Mind indentation when looking for source code JuliaDocs/Documenter.jl#684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix length calculation of broadcast over tuples #23887

Fix length calculation of broadcast over tuples #23887

pabloferz commented Sep 27, 2017

Sacha0 commented Sep 27, 2017

Sacha0 Sep 27, 2017

pabloferz Sep 27, 2017

Sacha0 Sep 27, 2017

pabloferz Sep 28, 2017

pabloferz commented Sep 27, 2017

Sacha0 commented Sep 27, 2017

pabloferz commented Sep 28, 2017

Sacha0 Sep 29, 2017

Sacha0 commented Sep 29, 2017

pabloferz commented Sep 29, 2017 •

edited

Loading

pabloferz commented Oct 2, 2017

TotalVerb Oct 2, 2017

pabloferz Oct 2, 2017 •

edited

Loading

TotalVerb Nov 7, 2017

pabloferz Nov 7, 2017 •

edited

Loading

TotalVerb Nov 7, 2017

TotalVerb commented Oct 3, 2017

timholy commented Oct 3, 2017

pabloferz commented Oct 3, 2017 •

edited

Loading

Sacha0 commented Oct 3, 2017

nanosoldier commented Oct 4, 2017

Sacha0 commented Oct 4, 2017

nanosoldier commented Oct 4, 2017

ararslan commented Nov 7, 2017

TotalVerb commented Nov 7, 2017

pabloferz commented Nov 7, 2017

Fix length calculation of broadcast over tuples #23887

Fix length calculation of broadcast over tuples #23887

Conversation

pabloferz commented Sep 27, 2017

Sacha0 commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloferz commented Sep 27, 2017

Sacha0 commented Sep 27, 2017

pabloferz commented Sep 28, 2017

Choose a reason for hiding this comment

Sacha0 commented Sep 29, 2017

pabloferz commented Sep 29, 2017 • edited Loading

pabloferz commented Oct 2, 2017

Choose a reason for hiding this comment

pabloferz Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloferz Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalVerb commented Oct 3, 2017

timholy commented Oct 3, 2017

pabloferz commented Oct 3, 2017 • edited Loading

Sacha0 commented Oct 3, 2017

nanosoldier commented Oct 4, 2017

Sacha0 commented Oct 4, 2017

nanosoldier commented Oct 4, 2017

ararslan commented Nov 7, 2017

TotalVerb commented Nov 7, 2017

pabloferz commented Nov 7, 2017

pabloferz commented Sep 29, 2017 •

edited

Loading

pabloferz Oct 2, 2017 •

edited

Loading

pabloferz Nov 7, 2017 •

edited

Loading

pabloferz commented Oct 3, 2017 •

edited

Loading