Limit broadcast mechanism over Nullables by p-zubieta · Pull Request #19787 · JuliaLang/julia

p-zubieta · 2016-12-31T00:13:28Z

This should address @JeffBezanson's concerns over #16961. That is broadcasting over Nullables treats them as containers only when mixed with scalars. If people need to work with arrays they can use comprehensions, e.g.

[x .+ 1 for x in [Nullable(1), Nullable(2)]]

If someone is relying heavily on arrays of Nullables they probably should be using NullableArrays instead.

CC. @TotalVerb

TotalVerb · 2016-12-31T00:18:58Z

I'm ok with this limitation to scalars, as was @johnmyleswhite in the other issue, at least for now. My understanding is that the lifting would apply to NullableArrays, and be implemented there, but not Array{T <: Nullable}.

cc @nalimilan @Sacha0 @ararslan

TotalVerb

Needs modification of NEWS.md and the manual, in the section on Nullables.

JeffBezanson · 2016-12-31T00:44:54Z

My hero! Thank you.

tkelman · 2016-12-31T00:52:31Z

   as a 1-dimensional array) expanding singleton dimensions.

-The following additional rules apply to `Nullable` arguments:
+The following additional rule apply to `Nullable` arguments:


rule applies

andreasnoack · 2016-12-31T01:00:06Z

This seems to simplify things so 👍. One thing that would be great to include in future design considerations for the broadcasting code would be extensibility beyond base. I.e. how user defined arrays should overload broadcast methods. I've been working on such a case in JuliaParallel/DistributedArrays.jl#120 and although it wasn't too hard with the current structure, it didn't feel like extensibility beyond base had been a priority.

tkelman · 2016-12-31T06:42:26Z

+# broadcast should only "peel" one container level
+let io = IOBuffer()
+    broadcast(x -> print(io, x), [Nullable(1.0)])
+    String(take!(io)) == "Nullable{Float64}(1.0)"


missing @test

tkelman · 2016-12-31T06:44:49Z

-_broadcast_type{S}(::Type{S}, f, T::Type, As...) = Base._return_type(f, typestuple(S, T, As...))
-_broadcast_type{T}(::Type{T}, f, A, Bs...) = Base._default_eltype(Base.Generator{ziptype(T, A, Bs...), ftype(f, A, Bs...)})
+_broadcast_type(f, T::Type, As...) = Base._return_type(f, typestuple(T, As...))
+_broadcast_type(f, A, Bs...) = Base._default_eltype(Base.Generator{ziptype(A, Bs...), ftype(f, A, Bs...)})


~~this is used a couple of times in the new sparse code that I suspect you'll have to change~~ done

tkelman · 2016-12-31T07:57:34Z

    fpreszeros = fofzeros == zero(fofzeros)
    maxnnzC = fpreszeros ? min(length(A), _sumnnzs(A, Bs...)) : length(A)
-    entrytypeC = Base.Broadcast._broadcast_type(Any, f, A, Bs...)
+    entrytypeC = Base.Broadcast._broadcast_type(f, A, Bs...)


(should be squashed on merge to avoid broken intermediate commits)

Already squashed it to prevent that.

nalimilan · 2016-12-31T14:41:04Z

-The following additional rules apply to `Nullable` arguments:
+The following additional rule applies to `Nullable` arguments:

 - If there is at least a `Nullable`, and all the arguments are scalars or


Remove the list since there's only one bullet.

nalimilan · 2016-12-31T14:54:47Z

+# broadcast should only "peel" one container level
+let io = IOBuffer()
+    broadcast(x -> print(io, x), [Nullable(1.0)])
+    @test String(take!(io)) == "Nullable{Float64}(1.0)"


Sounds a bit weird to use string comparison to test the type of the result. Why not e.g. call push! to add values to a vector instead?

p-zubieta · 2016-12-31T20:46:06Z

I think this is ready, unless there are more comments/concerns.

TotalVerb · 2016-12-31T20:46:43Z

Thanks for doing this work. This still needs an update to doc/src/manual/types.md.

Sacha0 · 2016-12-31T20:49:28Z

I think this is ready, unless there are more comments/concerns.

Hoping to review shortly (beginning within the next fifteen minutes). Best!

TotalVerb · 2016-12-31T21:10:46Z

@@ -291,22 +282,21 @@ ftype(T::Type, A...) = Type{T}
 # if the first argument is Any, then Nullable should be treated like a


Delete this comment.

TotalVerb · 2016-12-31T21:13:21Z

@@ -9,6 +9,8 @@ using Base: promote_eltype_op, linearindices, tail, OneTo, to_shape,
 import Base: broadcast, broadcast!


I think the is_nullable_array import can be removed, and probably the function itself too (in base/nullable.jl).

TotalVerb · 2016-12-31T21:13:47Z


 Broadcasts the arrays, tuples, `Ref`, nullables, and/or scalars `As` to a
 container of the appropriate type and dimensions. In this context, anything
-that is not a subtype of `AbstractArray`, `Ref` (except for `Ptr`s) or `Tuple`,


I don't think Nullable should be removed from this paragraph.

TotalVerb

LGTM. Thanks again!

Sacha0 · 2016-12-31T21:48:12Z

@@ -349,7 +325,7 @@ end

 Broadcasts the arrays, tuples, `Ref`, nullables, and/or scalars `As` to a


Not from this pull request, but should `Ref` be plural `Ref`s for consistency?

Sacha0 · 2016-12-31T21:52:31Z

 Broadcasts the arrays, tuples, `Ref`, nullables, and/or scalars `As` to a
 container of the appropriate type and dimensions. In this context, anything
-that is not a subtype of `AbstractArray`, `Ref` (except for `Ptr`s) or `Tuple`,
+that is not a subtype of `AbstractArray`, `Ref` (except for `Ptr`s), `Tuple`


(Serial/Oxford) comma after Tuple?

Sacha0 · 2016-12-31T21:56:00Z

@@ -359,16 +335,9 @@ the following rules:
   (and treats any `Ref` as a 0-dimensional array of its contents and any tuple
   as a 1-dimensional array) expanding singleton dimensions.


Not from this pull request, but perhaps "If there is at least an array or a Ref in the arguments" -> "If there is at least one array or Ref in the arguments"? Similarly, perhaps "and treats any Ref as a 0-dimensional array of its contents and any tuple as a 1-dimensional array" -> "treating Refs as 0-dimensional arrays, and tuples as 1-dimensional arrays"?

Edit: Perhaps "If the arguments contain at least one array or Ref, it returns an array, treating Refs as 0-dimensional arrays, tuples as 1-dimensional arrays, and expanding singleton dimensions." would be better still?

Sacha0 · 2016-12-31T21:59:31Z

- - If there is a tuple and a nullable, the result is an error, as this case is
-   not currently supported.
+The following additional rule applies to `Nullable` arguments: If there is at
+least a `Nullable`, and all the arguments are scalars or `Nullable`, it returns


Perhaps "at least a" should be "at least one"?

Sacha0 · 2016-12-31T22:05:47Z

    @test isa(aa .* aa', Array19745)
 end
+
+# broadcast should only "peel off" one container layer


A simpler way to test this behavior might be nice if you can think of one?

get.([Nullable(1), Nullable(2)]) == [1, 2] will work as a simpler test.

Unfortunately I don't have much time for looking for better/simpler tests for today. Maybe we could revisit them another occasion?

If @TotalVerb's suggestion does not suffice, sounds good on this end. Best!

That should work fine. Test added.

Sacha0

I am in no way qualified to provide feedback on the Nullable behavior change's desirability, but this pull request's mechanics LGTM! Thanks @pabloferz!

tkelman · 2017-01-01T00:50:38Z

-   as a 1-dimensional array) expanding singleton dimensions.
+ - If the arguments contain at least one array or `Ref`, it returns an array
+   (expanding singleton dimensions), and treats `Ref`s as 0-dimensional arrays,
+   and tuples as a 1-dimensional arrays.


tuples as 1-dimensional arrays

(can ci skip this if you do make check-whitespace locally)

dimensional, not element, sorry - just drop the a now that arrays is plural

tkelman · 2017-01-01T01:59:37Z

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

nanosoldier · 2017-01-01T05:40:17Z

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

tkelman · 2017-01-01T13:45:13Z

Looks like that i686 assertion failure "Cannot define a symbol twice!" in the reflection test is repeatable and has been happening since I merged this. Similar to #19792? I'll propose dropping back to LLVM 3.7 for now as a band-aid.

This reverts commit f147aaa.

tkelman · 2017-01-02T11:04:54Z

I don't get why nanosoldier here said "no regressions" but when run on the merged version against its immediate parent commit there were some big regressions - https://github.com/JuliaCI/BaseBenchmarkReports/blob/a1fd436b6823d8e27c7bc1cf3be1b8c1f212bde7/f147aaa_vs_b2ae5a5/report.md

I guess the intervening couple of commits that were merged to master between the time the PR run started and this was merged interacted badly here?

p-zubieta · 2017-01-02T14:44:00Z

That's strange, the code in this PR does not touch any code related to those regressions.

TotalVerb · 2017-01-02T20:47:50Z

I too don't see how a change to broadcasting can possibly affect comprehensions.

TotalVerb suggested changes Dec 31, 2016

View reviewed changes

tkelman added the missing data Base.missing and related functionality label Dec 31, 2016

andreasnoack mentioned this pull request Dec 31, 2016

Define general broadcasting of DArrays JuliaParallel/DistributedArrays.jl#120

Merged

tkelman reviewed Dec 31, 2016

View reviewed changes

Sacha0 mentioned this pull request Dec 31, 2016

Simplify and extend broadcast eltype promotion mechanism #19723

Merged

p-zubieta force-pushed the pz/bc-nullable branch from 0c7bd1a to ce7ebeb Compare December 31, 2016 06:31

tkelman reviewed Dec 31, 2016

View reviewed changes

nalimilan reviewed Dec 31, 2016

View reviewed changes

p-zubieta added 2 commits December 31, 2016 13:54

Limit broadcast mechanism over Nullables

e0f1cb1

Test that broadcast doesn't propagate too much

e5c270a

p-zubieta force-pushed the pz/bc-nullable branch from cb5afa5 to e5c270a Compare December 31, 2016 19:55

[ci skip] Update manual section on Nullables

f3aec3b

TotalVerb suggested changes Dec 31, 2016

View reviewed changes

Sacha0 mentioned this pull request Dec 31, 2016

RFC: Nullables as collections #16961

Merged

Address @TotalVerb comments

dcba165

TotalVerb approved these changes Dec 31, 2016

View reviewed changes

Sacha0 reviewed Dec 31, 2016

View reviewed changes

Sacha0 approved these changes Dec 31, 2016

View reviewed changes

Sacha0 mentioned this pull request Dec 31, 2016

broadcast[!] over combinations of scalars and sparse vectors/matrices #19724

Merged

Address @Sacha0 comments

afdd247

tkelman reviewed Jan 1, 2017

View reviewed changes

tkelman merged commit f147aaa into JuliaLang:master Jan 1, 2017

tkelman mentioned this pull request Jan 1, 2017

temporarily disable LLVM assertions on travis #19803

Closed

tkelman added a commit that referenced this pull request Jan 1, 2017

Revert "Limit broadcast mechanism over Nullables (#19787)"

830d8c9

This reverts commit f147aaa.

p-zubieta deleted the pz/bc-nullable branch January 5, 2017 23:50

TotalVerb mentioned this pull request Jan 10, 2017

remove some links to Nullable and methods #19961

Merged

Sacha0 mentioned this pull request Feb 22, 2017

Wishlist: API and documentation for extending broadcast #20740

Closed

		@@ -291,22 +282,21 @@ ftype(T::Type, A...) = Type{T}
		# if the first argument is Any, then Nullable should be treated like a

		@@ -9,6 +9,8 @@ using Base: promote_eltype_op, linearindices, tail, OneTo, to_shape,
		import Base: broadcast, broadcast!

		@@ -349,7 +325,7 @@ end

		Broadcasts the arrays, tuples, `Ref`, nullables, and/or scalars `As` to a

		@@ -359,16 +335,9 @@ the following rules:
		(and treats any `Ref` as a 0-dimensional array of its contents and any tuple
		as a 1-dimensional array) expanding singleton dimensions.

Uh oh!

Conversation

p-zubieta commented Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TotalVerb commented Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TotalVerb left a comment

Choose a reason for hiding this comment

Uh oh!

JeffBezanson commented Dec 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreasnoack commented Dec 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkelman Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-zubieta commented Dec 31, 2016

Uh oh!

TotalVerb commented Dec 31, 2016

Uh oh!

Sacha0 commented Dec 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TotalVerb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sacha0 Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sacha0 left a comment

Choose a reason for hiding this comment

Uh oh!

tkelman Jan 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkelman commented Jan 1, 2017

Uh oh!

nanosoldier commented Jan 1, 2017

Uh oh!

p-zubieta commented Dec 31, 2016 •

edited

Loading

TotalVerb commented Dec 31, 2016 •

edited

Loading

tkelman Dec 31, 2016 •

edited

Loading

TotalVerb left a comment •

edited

Loading

Sacha0 Dec 31, 2016 •

edited

Loading

tkelman Jan 1, 2017 •

edited

Loading