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.

Sign up for GitHub

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

RFC: Nullables as collections #16961

Merged
merged 10 commits into from
Dec 29, 2016
Merged

RFC: Nullables as collections #16961

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dbc4693
map and filter for Nullable
TotalVerb Jun 13, 2016
aa655eb
Scalar nullable broadcast
TotalVerb Nov 29, 2016
d3e536c
Vector broadcasting
TotalVerb Dec 20, 2016
74ccb05
Deal with nullable scalar in more general way
TotalVerb Dec 27, 2016
06b8297
Simplify broadcast_indices methods
TotalVerb Dec 27, 2016
242b333
Docs for broadcast
TotalVerb Dec 27, 2016
5d5de20
Fix broadcast_type breakage
TotalVerb Dec 27, 2016
2b7d457
Make tests test type
TotalVerb Dec 27, 2016
2f3d5cb
Use Any and Array instead of custom types
TotalVerb Dec 27, 2016
aab9f05
Add note about what is broken
TotalVerb Dec 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Simplify broadcast_indices methods
  • Loading branch information
@TotalVerb
TotalVerb committed Dec 27, 2016
commit 06b829746548b68b0ef5003f705ec80763b2248a
3 changes: 1 addition & 2 deletions base/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,8 @@ broadcast_indices() = ()
broadcast_indices(A) = broadcast_indices(containertype(A), A)
broadcast_indices(::Union{Type{Any}, Type{Nullable}}, A) = ()
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, Nullable is treated as any scalar here. So why do we need a specific method? Same below for Array{Nullable} and _broadcast_getindex.

Copy link
Contributor Author

@TotalVerb TotalVerb Dec 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The type information is computed assuming Nullable is a scalar, but that information is not fully propagated through. Perhaps it should be?

Copy link
Member

@nalimilan nalimilan Dec 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure what this means, I'm not really familiar with that code. @martinholters @pabloferz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean that nullable is only treated as a scalar as a special case.

broadcast_indices(::Type{Tuple}, A) = (OneTo(length(A)),)
broadcast_indices(::Type{Array}, A) = indices(A)
broadcast_indices(::Type{Array}, A::Ref) = ()
broadcast_indices(::Union{Type{Array}, Type{Array{Nullable}}}, A) = indices(A)
broadcast_indices{T<:Array}(::Type{T}, A) = indices(A)
@inline broadcast_indices(A, B...) = broadcast_shape((), broadcast_indices(A), map(broadcast_indices, B)...)
# shape (i.e., tuple-of-indices) inputs
broadcast_shape(shape::Tuple) = shape
Expand Down