Use new broadcast API #348

pablosanjose · 2017-12-07T19:07:19Z

Attempt to fix #347

(Please review carefully, as I'm still quite new at this.)

I implemented a StaticArrayStyle{N} <: AbtractArrayStyle{N} to plug into the new broadcast API introduced in julialang/#23939, together with two specialised indirections for broadcast and broadcast! that call the old @generated _broadcast and @generated _broadcast! routines. The latter does still seem to be a bit faster and allocate a little less than the Base broadcast! fallback. However, the Base fallback also works.

~~I see tight @code_native output, similar or identical to the one before julialang/#23939. Timings and allocations also seem identical, although I didn't do exhaustive testing.~~ ~~[EDIT: not always]~~ [EDIT2: fixed now, hopefully]

Caveat
The package tests now work again, except for those involving broadcast between Scalar(...) and normal Arrays. I'm afraid I need a bit of help with this one. As an example Scalar(3) .* [1,2,3] fails with the same error as Scalar(3) * 2, a BoundsError. This is the case both before and after julialang/#23939, but for some reason Scalar(3) .* [1,2,3] did work with the old broadcast API, and returned Array{Int}[2, 4, 6]. What is the expected behavior for this? What should Scalar(3) * 2 give? How should Scalar(3) .* [1,2,3] be fixed with the new broadcast API?

[EDIT: fixed Scalar broadcast. I needed to properly define the correct broadcast promotions using binary methods for BroadcastStyle]

This reverts commit 098dcbb.

pablosanjose · 2017-12-07T20:33:49Z

This still needs some work. The @code_native output is not as good as before. I'm not completely sure whether this is overhead from the new broadcast machinery or a faulty implementation on my part...

julia> f() = SVector(0,2,3) .* SVector(1,2,3)
f (generic function with 1 method)

Before:

julia> @btime f()
  1.345 ns (0 allocations: 0 bytes)
3-element SVector{3,Int64}:
 0
 4
 9

After:

julia> @btime f()
  3.712 ns (0 allocations: 0 bytes)
3-element SVector{3,Int64}:
 0
 4
 9

coveralls · 2017-12-07T20:46:21Z

Coverage decreased (-0.07%) to 93.297% when pulling 1a00c3f on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

codecov-io · 2017-12-07T20:46:22Z

Codecov Report

Merging #348 into master will decrease coverage by 0.06%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
- Coverage   93.36%   93.29%   -0.07%     
==========================================
  Files          37       37              
  Lines        2713     2715       +2     
==========================================
  Hits         2533     2533              
- Misses        180      182       +2

Impacted Files	Coverage Δ
src/broadcast.jl	`97.72% <80%> (-2.28%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb51021...5d5bab6. Read the comment docs.

pablosanjose · 2017-12-07T21:34:20Z

Aha. The problem was that specialising broadcast(f, ::StaticArrayStyle, ::Type{ElType}, inds::Tuple, As...) was unnecessarily slow, because both ElType and inds where computed with the generic combine_eltypes and combine_indices from Base. Those where not used, as the old method relied on the static StaticArray.broadcast_sizes methods. Specialising on broadcast(f, ::StaticArrayStyle, ::Void, ::Void, As...) instead gets us back to the old timings:

julia> @btime f()
  1.382 ns (0 allocations: 0 bytes)
3-element SVector{3,Int64}:
 0
 4
 9

coveralls · 2017-12-07T22:10:04Z

Coverage decreased (-0.07%) to 93.297% when pulling 1a00c3f on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

coveralls · 2017-12-08T00:41:32Z

Coverage decreased (-0.07%) to 93.297% when pulling 1a00c3f on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

coveralls · 2017-12-08T01:48:09Z

Coverage decreased (-0.07%) to 93.297% when pulling 75dc71e on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

coveralls · 2017-12-08T02:37:27Z

Coverage decreased (-0.07%) to 93.297% when pulling 2edd21e on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

coveralls · 2017-12-08T03:19:34Z

Coverage decreased (-0.07%) to 93.297% when pulling b146f8a on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

andyferris · 2017-12-08T05:14:02Z

src/broadcast.jl

+    BroadcastStyle(::Type{<:StaticArray{D, T, N}}) where {D, T, N} = StaticArrayStyle{N}()
+
+    # Fix Precedence: Make StaticArray - Array -> Array
+    BroadcastStyle(::StaticArrayStyle{M}, ::Broadcast.VectorStyle) where {N,M} = Broadcast.Unknown()


N is undefined.

Thanks Andy!

andyferris · 2017-12-08T05:15:54Z

src/broadcast.jl

+    BroadcastStyle(::StaticArrayStyle{M}, ::Broadcast.VectorStyle) where {N,M} = Broadcast.Unknown()
+    BroadcastStyle(::StaticArrayStyle{M}, ::Broadcast.MatrixStyle) where {N,M} = Broadcast.Unknown()
+
+    # Add a broadcast method that calls the old @generated routine


"old" isn't really necessary here

coveralls · 2017-12-08T17:27:40Z

Coverage decreased (-0.07%) to 93.297% when pulling 5d5bab6 on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

timholy

Looks quite good, thanks for tackling this!

timholy · 2017-12-10T17:02:56Z

src/broadcast.jl

+
+    # Fix Precedence: Make StaticArray - Array -> Array
+    BroadcastStyle(::StaticArrayStyle{M}, ::Broadcast.VectorStyle) where M = Broadcast.Unknown()
+    BroadcastStyle(::StaticArrayStyle{M}, ::Broadcast.MatrixStyle) where M = Broadcast.Unknown()


VectorStyle and MatrixStyle are unfortunate (and hopefully temporary) objects because we don't trust that sparse broadcasting generalizes correctly beyond Array (see JuliaLang/julia#23939 (comment)). However, this suggests we'll have to support them throughout julia 1.x. 😢

Long-term the one we want to implement would be with DefaultArrayStyle. Instead of returning Unknown, it might be best to do something like this (untested):

BroadcastStyle(::StaticArrayStyle{M}, ::Broadcast.DefaultArrayStyle{N}) where {M,N} = DefaultArrayStyle(Broadcast._max(Val(M), Val(N))

Hey Tim, thanks for reviewing! Your designs are a pleasure to work with.

So you suggest including this rule, in addition to the VectorStyle and MatrixStyle ones, to take over once the latter are removed, yes? Done.

Right. Also the DefaultArrayStyle is necessary even now for dimensions higher than 2.

Thanks for the kind words about the API. I think it's also fair to say that unfortunately they don't help StaticArrays that much; they're more oriented around improving the API for mutable arrays. But at least it's documented, and hopefully that's worth something.

Really grateful to you for tackling this!

coveralls · 2017-12-10T20:55:12Z

Coverage decreased (-0.07%) to 93.297% when pulling b70fae3 on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

coveralls · 2017-12-10T23:01:48Z

Coverage decreased (-0.07%) to 93.297% when pulling d7b23dd on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

pablosanjose · 2017-12-18T13:33:32Z

Please forgive the supernoob question: what is that failed check from coveralls? Is it just because I need to add some tests? (I'm not sure if this is waiting for me to do something...)

tkoolen · 2018-01-10T20:51:54Z

Note that there have been additional changes to broadcast! recently (see JuliaLang/julia#24992), which should enable resolution of your TODO. See https://docs.julialang.org/en/latest/manual/interfaces/#extending-in-place-broadcast-1 for the required method signatures.

Re: coveralls: a slight decrease in code coverage is to be expected. Code coverage is determined on Julia 0.6, so the 0.7 code doesn't get run. It doesn't matter (check is not required to pass before merging).

tkoolen · 2018-01-10T21:35:10Z

Actually, here's a PR against your branch that implements the change: https://github.com/pablosanjose/StaticArrays.jl/pull/1.

timholy · 2018-01-10T22:06:39Z

And probably more changes coming...JuliaLang/julia#25377

Adapt to new broadcast! implementation in Base.

coveralls · 2018-01-15T22:07:57Z

Coverage decreased (-0.07%) to 93.297% when pulling 0ccfc43 on pablosanjose:newbroadcast into bb51021 on JuliaArrays:master.

andyferris · 2018-01-17T06:06:13Z

Sorry, I haven't got around to this in a while. Is this good to go?

pablosanjose · 2018-01-17T07:15:53Z

It is as far as I'm concerned... (At least until JuliaLang/julia#25377 rains hellfire on us! :-P)

pablosanjose · 2018-01-19T16:00:59Z

I just tried this again in the latest Julia master, and I think there are still broadcast problems.

julia> @SVector[1,2,3]*2
ERROR: UndefVarError: Inference not defined
Stacktrace:
 [1] @generated body at /Users/pablo/.julia/v0.7/StaticArrays/src/broadcast.jl:152 [inlined]
 [2] _broadcast at /Users/pablo/.julia/v0.7/StaticArrays/src/broadcast.jl:94 [inlined]
 [3] broadcast at /Users/pablo/.julia/v0.7/StaticArrays/src/broadcast.jl:62 [inlined]
 [4] broadcast at ./broadcast.jl:615 [inlined]
 [5] *(::SVector{3,Int64}, ::Int64) at /Users/pablo/.julia/v0.7/StaticArrays/src/linalg.jl:26
 [6] top-level scope

Not sure if this happened some weeks ago. I'll try to look into it if I can find the time.

andyferris · 2018-01-21T04:23:17Z

FYI @pablosanjose I've been working on a branch which deals with all the v0.7 carnage and have incorporated these changes there (thanks!).

However every day I nearly get the tests passing and then the next day something else breaks. (This time it's Base.LinAlg -> LinearAlgebra stdlib change. Fun.)

pablosanjose · 2018-01-21T08:25:31Z

You have all my support and empathy. StaticArrays is a central tool for many of us. Keep at it!

pablosanjose added 7 commits December 7, 2017 17:09

first version

ec78329

wip

0b7bab8

wip (disabled broadcast!)

1c21255

patched broadcast!, added version check

a2995d5

deleted old broadcast.jl

3ea9ad1

WIP: trying to fix Scalar broadcast

098dcbb

Revert "WIP: trying to fix Scalar broadcast"

48a84b0

This reverts commit 098dcbb.

pablosanjose mentioned this pull request Dec 7, 2017

ERROR: MethodError: no method matching result_style(::Type{StaticArray}) #347

Closed

pablosanjose added 3 commits December 7, 2017 20:17

Whitespace

d6b58f4

Reverted use of uninitialised for compatibility

1a00c3f

fix Scalar broadcast, array precedence

75dc71e

pablosanjose added 2 commits December 7, 2017 21:50

Less binary BroadcastStyle methods

2edd21e

Avoid calling dynamic combine_indices (duh!)

b146f8a

andyferris reviewed Dec 8, 2017

View reviewed changes

Addressed points by andyferris

5d5bab6

timholy reviewed Dec 10, 2017

View reviewed changes

timholy mentioned this pull request Dec 10, 2017

Rework the broadcast API and document it JuliaLang/julia#23939

Merged

2 tasks

add a DefaultArrayStyle rule

b70fae3

edited comments, added reminders to fix VectorStyle, MatrixStyle

d7b23dd

timholy mentioned this pull request Dec 21, 2017

RFC: introduce runtime representation of broadcast fusion JuliaLang/julia#23692

Closed

Adapt to new broadcast! implementation in Base.

35969d7

Merge pull request #1 from tkoolen/tk/newbroadcast

0ccfc43

Adapt to new broadcast! implementation in Base.

pablosanjose mentioned this pull request Jan 19, 2018

Fix Random breakage and deprecations for v0.7 #360

Closed

pablosanjose closed this Jan 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new broadcast API #348

Use new broadcast API #348

pablosanjose commented Dec 7, 2017 •

edited

Loading

pablosanjose commented Dec 7, 2017 •

edited

Loading

coveralls commented Dec 7, 2017

codecov-io commented Dec 7, 2017 •

edited

Loading

pablosanjose commented Dec 7, 2017

coveralls commented Dec 7, 2017

coveralls commented Dec 8, 2017

coveralls commented Dec 8, 2017

coveralls commented Dec 8, 2017

coveralls commented Dec 8, 2017

andyferris Dec 8, 2017

pablosanjose Dec 8, 2017

andyferris Dec 8, 2017

coveralls commented Dec 8, 2017

timholy left a comment

timholy Dec 10, 2017

pablosanjose Dec 10, 2017

timholy Dec 11, 2017

coveralls commented Dec 10, 2017

coveralls commented Dec 10, 2017

pablosanjose commented Dec 18, 2017

tkoolen commented Jan 10, 2018

tkoolen commented Jan 10, 2018

timholy commented Jan 10, 2018

coveralls commented Jan 15, 2018

andyferris commented Jan 17, 2018

pablosanjose commented Jan 17, 2018

pablosanjose commented Jan 19, 2018

andyferris commented Jan 21, 2018

pablosanjose commented Jan 21, 2018

Use new broadcast API #348

Use new broadcast API #348

Conversation

pablosanjose commented Dec 7, 2017 • edited Loading

pablosanjose commented Dec 7, 2017 • edited Loading

coveralls commented Dec 7, 2017

codecov-io commented Dec 7, 2017 • edited Loading

Codecov Report

pablosanjose commented Dec 7, 2017

coveralls commented Dec 7, 2017

coveralls commented Dec 8, 2017

coveralls commented Dec 8, 2017

coveralls commented Dec 8, 2017

coveralls commented Dec 8, 2017

andyferris Dec 8, 2017

Choose a reason for hiding this comment

pablosanjose Dec 8, 2017

Choose a reason for hiding this comment

andyferris Dec 8, 2017

Choose a reason for hiding this comment

coveralls commented Dec 8, 2017

timholy left a comment

Choose a reason for hiding this comment

timholy Dec 10, 2017

Choose a reason for hiding this comment

pablosanjose Dec 10, 2017

Choose a reason for hiding this comment

timholy Dec 11, 2017

Choose a reason for hiding this comment

coveralls commented Dec 10, 2017

coveralls commented Dec 10, 2017

pablosanjose commented Dec 18, 2017

tkoolen commented Jan 10, 2018

tkoolen commented Jan 10, 2018

timholy commented Jan 10, 2018

coveralls commented Jan 15, 2018

andyferris commented Jan 17, 2018

pablosanjose commented Jan 17, 2018

pablosanjose commented Jan 19, 2018

andyferris commented Jan 21, 2018

pablosanjose commented Jan 21, 2018

pablosanjose commented Dec 7, 2017 •

edited

Loading

pablosanjose commented Dec 7, 2017 •

edited

Loading

codecov-io commented Dec 7, 2017 •

edited

Loading