Fix several broadcast issues (fixes #197, #199, #200, #242) #274

wsshin · 2017-08-08T18:11:19Z

This PR fixes several issues raised for broadcast on SArray. Specifically,

it broadcasts properly between SArray and RowVec{<:Any,SVector} to generate an Sarray (fixes Broadcast inconsistency with Base #197, fixes Broadcast should handle transpose of StaticVector #200, and fixes broadcast between SMatrix{M,N} and SMatrix{M,1} (or SMatrix{1,N}) doesn't work #242), and
it also produces a result consistent with Base's for zero-length SArary (fixes Another broadcast inconsistency with Base #199).
Additionally, it replaces Base.promote_op with Core.Inference.returnt_type, because the former infers a wrong return type of an operation when the input arguments of the operation are abstract (see https://discourse.julialang.org/t/alternatives-to-base-promote-op-op-type-for-op-type/5289/6).

coveralls · 2017-08-08T19:13:02Z

Coverage increased (+0.01%) to 90.492% when pulling 3a49b3a on wsshin:broadcast into 7d711ab on JuliaArrays:master.

codecov-io · 2017-08-08T19:13:40Z

Codecov Report

Merging #274 into master will increase coverage by 0.29%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
+ Coverage   90.66%   90.96%   +0.29%     
==========================================
  Files          36       36              
  Lines        2658     2666       +8     
==========================================
+ Hits         2410     2425      +15     
+ Misses        248      241       -7

Impacted Files	Coverage Δ
src/mapreduce.jl	`100% <100%> (ø)`	⬆️
src/broadcast.jl	`100% <100%> (ø)`	⬆️
src/linalg.jl	`97.39% <0%> (+0.01%)`	⬆️
src/eigen.jl	`93.72% <0%> (+0.11%)`	⬆️
src/abstractarray.jl	`90.32% <0%> (+11.29%)`	⬆️

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 7d711ab...9cf3d2a. Read the comment docs.

coveralls · 2017-08-08T20:40:08Z

Coverage increased (+0.01%) to 90.492% when pulling e45d995 on wsshin:broadcast into 7d711ab on JuliaArrays:master.

coveralls · 2017-08-08T21:44:46Z

Coverage increased (+0.01%) to 90.492% when pulling 815860a on wsshin:broadcast into 7d711ab on JuliaArrays:master.

…oadcast

wsshin · 2017-08-08T22:11:00Z

Sorry about the duplicate commits. I pulled from master and rebased on master after creating this PR, and ended up having several duplicate commits.

wsshin · 2017-08-08T22:21:41Z

A part of the reason I pushed several commits is that I thought I could address #198 as well by using Base.promote_op instead of Core.Inference.return_type, because the former seemed to infer a function's return type more specifically when the function's input arguments are abstract. This turned out to be an illusion; see the discussion in https://discourse.julialang.org/t/alternatives-to-base-promote-op-op-type-for-op-type/5289/6. So, I decided to use Core.Inference.return_type and give up on addressing #198.

Ultimately, I think we need to promote types element-by-element, as discussed in JuliaLang/julia#19669. Julia 0.6 seems to support this already (even though the mentioned issue is still open), but I haven't looked into how it does in detail.

coveralls · 2017-08-08T23:57:41Z

Coverage increased (+0.01%) to 90.492% when pulling 8a265e8 on wsshin:broadcast into 7d711ab on JuliaArrays:master.

coveralls · 2017-08-09T01:04:01Z

Coverage increased (+0.09%) to 90.567% when pulling b98baa7 on wsshin:broadcast into 7d711ab on JuliaArrays:master.

andyferris · 2017-08-09T05:52:36Z

Very nice - thanks :)

Does this preserve RowVectorness? We should reproduce behavior similar to this in Base:

julia 0.6> 1 .+ [1,2]'
1×2 RowVector{Int64,Array{Int64,1}}:
 2  3

julia 0.6> [1,2]' .* [2,3]'
1×2 RowVector{Int64,Array{Int64,1}}:
 2  6

(Sorry, don't have a REPL to check this out right now)

wsshin · 2017-08-09T12:57:11Z

Yes it does:

julia> 1 .+ SVector(1,2)'
1×2 RowVector{Int64,SVector{2,Int64}}:
 2  3

julia> SVector(1,2)' .* SVector(2,3)'
1×2 RowVector{Int64,SVector{2,Int64}}:
 2  6

However, I didn't intend this behavior to be honest.

wsshin · 2017-08-10T07:01:49Z

I think now I know why my code does not disturb RowVectorness.

Whenever some operation is broadcasted between Union{Number,RowVector} type arguments such that the output is expected to be a RowVector, the following function defined here is called first:

@inline broadcast(f, rowvecs::Union{Number,RowVector}...) =
    RowVector(broadcast(transpose∘f, to_vecs(rowvecs...)...))

This basically strips all the RowVector{...} from the arguments, broadcasts the operation, and then puts RowVector{...} back to the result. Therefore in this case broadcast_c of the StaticArrays package does not see RowVectors at all and only sees StaticArrays as its arguments. In other words, the following code I added

_containertype(::Type{<:RowVector{<:Any,<:SVector}}) = StaticArray
@inline broadcast_sizes(a::RowVector{<:Any,<:SVector}, as...) = (Size(a), broadcast_sizes(as...)...)

does not play any role in this procedure and preserves RowVectorness.

However, the added code plays role when an operation is broadcasted between RowVectors and StaticArrays such that the output is expected to be a StaticArray rather than a RowVector. In this case, because not all the arguments are Union{Number,RowVector} type, broadcast is directly applied without striping RowVector{...}. Then broadcast_c of the StaticArrays package sees RowVectors together with StaticArrays as its arguments. The code I added treats these RowVectors as StaticArrays and perform broadcast to produce a StaticArray output.

c42f

This makes sense to me, thanks for fixing things :-)

I've added a couple of comments for consideration of adding a few minor niceties.

c42f · 2017-08-10T07:14:43Z

test/broadcast.jl

-        @test_broken @inferred(broadcast(+, m1, m2)) === @SMatrix [2 6; 4 8] #197
-        @test_broken @inferred(m1 .+ m2) === @SMatrix [2 6; 4 8] #197
+        @test @inferred(broadcast(+, m1, m2)) === @SMatrix [2 6; 4 8] #197
+        @test @inferred(m1 .+ m2) === @SMatrix [2 6; 4 8] #197


Perhaps it makes sense to remove the issue reference from these individually, and instead describe the behaviour which is being tested in a short comment. (That comment could include the issue crossref instead.)

In my opinion the source code (including comments) should be the source of truth about design decisions and the reasons for testing given behaviour: the source is a living document, but issues become abandoned as soon as they're closed.

c42f · 2017-08-10T07:18:11Z

src/mapreduce.jl

@@ -235,7 +235,7 @@ end
 @generated function _diff(::Size{S}, a::StaticArray, ::Type{Val{D}}) where {S,D}
    N = length(S)
    Snew = ([n==D ? S[n]-1 : S[n] for n = 1:N]...)
-    T = Base.promote_op(-, eltype(a), eltype(a))
+    T = Core.Inference.return_type(-, Tuple{eltype(a),eltype(a)})


IIRC calling Inference.return_type is considered a bit of a dirty trick by the core devs ;-) If used, it can change the behaviour of code depending on optimizations to the julia inference engine. (Is this the reason to avoid it? Perhaps there's more and someone can enlighten me?).

I know we do it in another couple of places in StaticArrays, but I think we can avoid it here by doing something like

T = typeof(one(eltype(a)) - one(eltype(a)))

or some similar such trick?

The version for _mapreducedim further up is more tricky, because it takes an arbitrary op in the reduction. Unlike diff, it's not clear that the data will be numeric or that calling one(eltype(a)) makes sense in that case.

wsshin · 2017-08-10T19:21:22Z

I agree with @c42f on both points, and made changes accordingly. Thanks for the comments!

coveralls · 2017-08-10T20:18:20Z

Coverage increased (+0.5%) to 90.96% when pulling 9cf3d2a on wsshin:broadcast into 7d711ab on JuliaArrays:master.

c42f · 2017-08-11T03:46:14Z

Very nice, thanks :-)

wsshin · 2017-08-11T04:14:24Z

Thanks for merging! I thought this merge would automatically close the Issues mentioned in the PR title, but it closed only the first one. I closed the last one that I opened. Could you close the middle two?

andyferris · 2017-08-11T06:33:13Z

In other words, the following code I added ... does not play any role in this procedure and preserves RowVectorness.

That's great news. :) Thanks for this.

wsshin · 2018-02-06T22:51:45Z

It seems that this PR hasn't been added to a release yet. I don't know how PRs to release are selected in general, but I'm curious if there are any reasons that prevent releasing this PR. One of my packages has several parts that can be simplified using the features implemented in this PR.

andyferris · 2018-02-06T23:25:03Z

Wow, it's been a while then!

I guess we can do a release? The v0.7 compatibility stuff is only half done and has made the code uglier in some places, but the tests all pass on v0.6.

wsshin · 2018-02-07T05:47:35Z

That will be great for me.

Also, is there an easy way to check if some PRs have been released or not? I just searched through the release descriptions for the PR numbers, but wonder if there is an easier way.

According to my search, my other merged PRs, #263 and #255, don't seem released yet, either...

mschauer · 2018-02-07T09:42:58Z

I checked my dependency https://github.com/mschauer/Bridge.jl against master and all is fine as well.

wsshin · 2018-02-28T16:33:38Z

If there is anything I could help in order to create a release with this PR, please let me know.

andyferris · 2018-02-28T22:52:01Z

Yes, sorry, I'll do it now. @wsshin it's easiest to do a "release" via github interface and let attobot do the rest.

Fix several broadcast issues

3a49b3a

Additionally fix JuliaArrays#198 by using Base.promote_op

e45d995

wsshin changed the title ~~Fix several broadcast issues (fixes #197, #199, #200, #242)~~ Fix several broadcast issues (fixes #197, #198, #199, #200, #242) Aug 8, 2017

Revert some @test for JuliaArrays#198 to @test_broken

815860a

wsshin changed the title ~~Fix several broadcast issues (fixes #197, #198, #199, #200, #242)~~ Fix several broadcast issues (fixes #197, #199, #200, #242) Aug 8, 2017

wsshin added 6 commits August 8, 2017 17:45

Fix several broadcast issues

6c94be9

Additionally fix JuliaArrays#198 by using Base.promote_op

c96363a

Revert some @test for JuliaArrays#198 to @test_broken

c0be60e

Revert Base.promote_op to Core.Inference.return_type

5b6665f

Merge branch 'broadcast' of github.com:wsshin/StaticArrays.jl into br…

b98baa7

…oadcast

Revert Base.promote_op to Core.Inference.return_type

8a265e8

c42f approved these changes Aug 10, 2017

View reviewed changes

wsshin added 2 commits August 10, 2017 15:18

Avoid using Core.Inference.return_type when possible

1fb2024

Use more descriptive comments in test/broadcast.jl

9cf3d2a

c42f merged commit 9779dd5 into JuliaArrays:master Aug 11, 2017

wsshin mentioned this pull request Aug 11, 2017

broadcast between SMatrix{M,N} and SMatrix{M,1} (or SMatrix{1,N}) doesn't work #242

Closed

wsshin deleted the broadcast branch August 11, 2017 04:15

This was referenced Aug 14, 2017

Another broadcast inconsistency with Base #199

Closed

Broadcast should handle transpose of StaticVector #200

Closed

This was referenced Mar 21, 2018

Improve StaticArrays usage stevengj/GeometryPrimitives.jl#27

Merged

Further simplify StaticArrays usage stevengj/GeometryPrimitives.jl#29

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix several broadcast issues (fixes #197, #199, #200, #242) #274

Fix several broadcast issues (fixes #197, #199, #200, #242) #274

wsshin commented Aug 8, 2017 •

edited

Loading

coveralls commented Aug 8, 2017

codecov-io commented Aug 8, 2017 •

edited

Loading

coveralls commented Aug 8, 2017

coveralls commented Aug 8, 2017

wsshin commented Aug 8, 2017

wsshin commented Aug 8, 2017

coveralls commented Aug 8, 2017

coveralls commented Aug 9, 2017

andyferris commented Aug 9, 2017

wsshin commented Aug 9, 2017 •

edited

Loading

wsshin commented Aug 10, 2017 •

edited

Loading

c42f left a comment

c42f Aug 10, 2017

c42f Aug 10, 2017

wsshin commented Aug 10, 2017

coveralls commented Aug 10, 2017

c42f commented Aug 11, 2017

wsshin commented Aug 11, 2017

andyferris commented Aug 11, 2017

wsshin commented Feb 6, 2018

andyferris commented Feb 6, 2018

wsshin commented Feb 7, 2018

mschauer commented Feb 7, 2018

wsshin commented Feb 28, 2018

andyferris commented Feb 28, 2018

Fix several broadcast issues (fixes #197, #199, #200, #242) #274

Fix several broadcast issues (fixes #197, #199, #200, #242) #274

Conversation

wsshin commented Aug 8, 2017 • edited Loading

coveralls commented Aug 8, 2017

codecov-io commented Aug 8, 2017 • edited Loading

Codecov Report

coveralls commented Aug 8, 2017

coveralls commented Aug 8, 2017

wsshin commented Aug 8, 2017

wsshin commented Aug 8, 2017

coveralls commented Aug 8, 2017

coveralls commented Aug 9, 2017

andyferris commented Aug 9, 2017

wsshin commented Aug 9, 2017 • edited Loading

wsshin commented Aug 10, 2017 • edited Loading

c42f left a comment

Choose a reason for hiding this comment

c42f Aug 10, 2017

Choose a reason for hiding this comment

c42f Aug 10, 2017

Choose a reason for hiding this comment

wsshin commented Aug 10, 2017

coveralls commented Aug 10, 2017

c42f commented Aug 11, 2017

wsshin commented Aug 11, 2017

andyferris commented Aug 11, 2017

wsshin commented Feb 6, 2018

andyferris commented Feb 6, 2018

wsshin commented Feb 7, 2018

mschauer commented Feb 7, 2018

wsshin commented Feb 28, 2018

andyferris commented Feb 28, 2018

wsshin commented Aug 8, 2017 •

edited

Loading

codecov-io commented Aug 8, 2017 •

edited

Loading

wsshin commented Aug 9, 2017 •

edited

Loading

wsshin commented Aug 10, 2017 •

edited

Loading