Fix broadcast issue #196
Fix broadcast issue #196
Conversation
|
I think we can get away with something a bit simpler than this. Here's what I had: @inline broadcast_sizes(a::StaticArray, as...) = (Size(a), broadcast_sizes(as...)...)
@inline broadcast_sizes(a::Number, as...) = (Size(), broadcast_sizes(as...)...)
@inline broadcast_sizes() = () |
|
Yeah, that is better. |
|
The failed test here isn't your fault, I restarted it. Do you want to finish this one off by cleaning up a little and adding tests? |
|
Yeah, I'm happy to do that. I'll make your change and add some tests. |
|
Awesome, thanks a lot! I had started messing with tests, but hadn't done much. |
…mit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # Date: Mon May 29 15:24:14 2017 +0200 # # On branch broadcast-fix # Your branch is ahead of 'origin/broadcast-fix' by 1 commit. # (use "git push" to publish your local commits) # # Changes to be committed: # modified: src/broadcast.jl # # Untracked files: # test/broadcast.jl #
|
Okay, so I've added a bunch of tests and found some issues/inconsistencies with broadcast from Base. I've listed those here #197, #198, #199 and #200 and I've added (commented out) test cases for them. Let me know if you'd like me to add anything else. Oh, I also made a small change to the |
test/broadcast.jl
Outdated
| @test sin.(α) === broadcast(sin, α) | ||
| @test sin.(3.2) === broadcast(sin, 3.2) == sin(3.2) | ||
| @test factorial.(3) === broadcast(factorial, 3) | ||
| @test atan2.(x, y) === broadcast(atan2, x, y) |
There was a problem hiding this comment.
I don't think we need all of these - julia's syntax lowering pass does the conversion between the dot notation and broadcast, and this package has no control over it.
| @@ -147,5 +146,6 @@ end | |||
| return quote | |||
| @_inline_meta | |||
| @inbounds $(Expr(:block, exprs...)) | |||
| return dest | |||
test/broadcast.jl
Outdated
| @test broadcast_sizes(ones(t), ones(t), 1) === (Size(t), Size(t), Size()) | ||
| @test broadcast_sizes(1, ones(t), 1) === (Size(), Size(t), Size()) | ||
| @test broadcast_sizes(ones(t), 1, 1) === (Size(t), Size(), Size()) | ||
| @test broadcast_sizes(1, 1, ones(t)) === (Size(), Size(), Size(t)) |
There was a problem hiding this comment.
Might be also worth checking that these are @inferred
test/broadcast.jl
Outdated
| @test broadcast_sizes(ones(t), 1, 1) === (Size(t), Size(), Size()) | ||
| @test broadcast_sizes(1, 1, ones(t)) === (Size(), Size(), Size(t)) | ||
| end | ||
| @test broadcast((a,b,c)->0, SVector(1,1), 0, 0) == SVector(0, 0) |
There was a problem hiding this comment.
Could do with a comment See #191 perhaps?
test/broadcast.jl
Outdated
| @testset "2x2 StaticMatrix with 1x2 StaticMatrix" begin | ||
| m1 = @SMatrix [1 2; 3 4] | ||
| m2 = @SMatrix [1 4] | ||
| # @test @inferred(broadcast(+, m1, m2)) === @SMatrix [2 6; 4 8] #197 |
There was a problem hiding this comment.
All the places where tests are currently commented out (because they're broken) you can use @test_broken instead. This runs the test and verifies that it is, indeed, broken - so we won't forget to add them back in later when these issues are fixed.
|
Great. Thanks for the feedback. I think I've made all the changes you suggested. |
…iaArrays#196) * implement specialization of Base.dataids to speed-up broadcast * dataids should return a tuple of UInts Co-authored-by: Pietro Vertechi <[email protected]> * add explicit test of dataids Co-authored-by: Pietro Vertechi <[email protected]>
Fix for #191