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

Use containertype to determine array type for array broadcast #19745

Merged
merged 1 commit into from
Dec 30, 2016
Merged

Conversation

andreasnoack
Copy link
Member

@andreasnoack andreasnoack commented Dec 28, 2016

When trying to get broadcast working for DArrays the line deleted in this PR caused some issues because it forced all broadcasting of AbstractArrays into Arrays. With the line deleted, this line is called instead and it allows for overloading containertype for DArrays.

cc: @pabloferz

@tkelman tkelman added the needs tests Unit tests are required for this change label Dec 28, 2016
@pabloferz
Copy link
Contributor

LGTM. This shouldn't affect performance, but I'd check anyway (you cannot be too cautious here).

@tkelman
Copy link
Contributor

tkelman commented Dec 29, 2016

should add a test so it doesn't regress again

@andreasnoack
Copy link
Member Author

@tkelman Do you mean testing that broadcast can be extended to a new AbstractArray?

@andreasnoack
Copy link
Member Author

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

@nanosoldier
Copy link
Collaborator

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

@tkelman
Copy link
Contributor

tkelman commented Dec 29, 2016

testing that broadcast can be extended to a new AbstractArray?

sure, if that's what having this line broke

@andreasnoack andreasnoack removed the needs tests Unit tests are required for this change label Dec 29, 2016
@test a .* a' == @inferred(aa .* aa')
@test isa(aa .+ 1, Array19745)
@test isa(aa .* aa', Array19745)

Copy link
Contributor

Choose a reason for hiding this comment

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

blank lines right after begin and right before end don't look very good and waste space imo, we don't format most testset blocks like this

@andreasnoack andreasnoack force-pushed the anj/bc branch 2 times, most recently from db674c3 to 30c29b0 Compare December 29, 2016 19:18
@@ -313,7 +301,7 @@ ziptype{T}(::Type{T}, A) = typestuple(T, A)
ziptype{T}(::Type{T}, A, B) = (Base.@_pure_meta; Iterators.Zip2{typestuple(T, A), typestuple(T, B)})
@inline ziptype{T}(::Type{T}, A, B, C, D...) = Iterators.Zip{typestuple(T, A), ziptype(T, B, C, D...)}

_broadcast_type{S}(::Type{S}, f, T::Type, As...) = Base._return_type(S, typestuple(S, T, As...))
_broadcast_type{S}(::Type{S}, f, T::Type, As...) = Base._return_type(f, typestuple(S, T, As...))
Copy link
Member Author

Choose a reason for hiding this comment

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

@TotalVerb I'm not sure what the purpose of the first type argument is but it seemed wrong to pass it as the first argument to _return_type and it also caused a test failure in the test at line 366 below. Does this change seem right to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

The first argument is used to differentiate when Nullable should be treated as scalar (when S === Any) or as container (when S === Array), but should be passed as an argument only for typestuple. This was certainly a mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this was a mistake.

@@ -10,21 +10,9 @@ import Base: broadcast, broadcast!
export bitbroadcast, dotview
export broadcast_getindex, broadcast_setindex!

## Broadcasting utilities ##

broadcast_array_type() = Array
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can avoid defining these if we delete the method in line 26 and just use this method which was the origin purpose of this PR.

Copy link
Contributor

@pabloferz pabloferz Dec 29, 2016

Choose a reason for hiding this comment

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

Yes, you can remove this, it should do no harm. But I believe the ## Broadcasting utilities ## is still useful for organizing the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, this was an annoying special case I had to add. Thanks for addressing it!

@andreasnoack
Copy link
Member Author

@TotalVerb This PR had conflicts with #16961 so please take a look and see if I've messed something up.

@stevengj
Copy link
Member

@andreasnoack, when you link to lines in source code on github, be sure to type y so that github gives you a link tied to a particular commit. Your source-code link at the top of this PR is broken by a recent update to broadcast.jl.

@andreasnoack
Copy link
Member Author

@stevengj I've updated the link

Copy link
Contributor

@TotalVerb TotalVerb left a comment

Choose a reason for hiding this comment

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

I am a little concerned about the difficulty in getting custom array types working with nullables, but that's only tangentially related to this PR. After the 0.6 release, we will probably have to move the Nullable handling out of promote_containertype, else risk an exponential explosion of special cases for that method.

This PR looks good. I was wondering about the purpose of the AbstractArray special case. Glad to see it's not needed.

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

+1, had this change in mind as well. (The line at issue forced (1) an ad hoc reimplementation of the broadcast container-type dispatch mechanism in #16961, (2) a similar workaround in code extending sparse broadcast to structured matrices that I haven't submitted yet, and (3) specific structuring of some of the sparse broadcast methods.) Best!

@tkelman
Copy link
Contributor

tkelman commented Dec 30, 2016

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

@KristofferC
Copy link
Member

@nanosoldier sleeping? @jrevels

@andreasnoack
Copy link
Member Author

Maybe a conflict is a problem for the soldier? I'll try again @nanosoldier runbenchmarks(ALL, vs = ":master")

@tkelman
Copy link
Contributor

tkelman commented Dec 30, 2016

was there one? last night's daily build didn't finish either, probably network error again?

@andreasnoack
Copy link
Member Author

A tiny one from #19722 because it also added a test at the bottom of test/broadcast.jl.

@tkelman
Copy link
Contributor

tkelman commented Dec 30, 2016

ah right. that wasn't there when I asked for nanosoldier an hour ago.

@jrevels
Copy link
Member

jrevels commented Dec 30, 2016

It was the getaddrinfo error again, merge conflicts shouldn't be a problem for Nanosoldier.

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

OT: I can't wait until v0.6 is released and the web stack is updated, #19699 will make it so much easier to debug network issues...

@nanosoldier
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants