Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

Rewrite broadcast() and map() based on lift() #166

Merged
merged 19 commits into from
Jan 23, 2017
+255 −645
Merged

Rewrite broadcast() and map() based on lift() #166

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4233744
Rewrite broadcast() using Base implementation and lifting semantics
nalimilan Nov 25, 2016
899ed75
Use generated function to avoid allocation
nalimilan Nov 26, 2016
23ec9f2
Fix performance without generated functions
nalimilan Nov 26, 2016
2953516
Improve efficiency of null checking for generic case
nalimilan Nov 26, 2016
82d9fde
Fix call to null_safe_op() for generic case
nalimilan Nov 26, 2016
cc787d9
Fix remaining allocations in generic method
nalimilan Nov 27, 2016
0d649dd
Add non-splatting methods up to 7 arguments
nalimilan Nov 27, 2016
6518c47
Also port map() to lift()
nalimilan Dec 4, 2016
a69bd5f
Fix broadcast() tests
nalimilan Dec 4, 2016
a1926c7
Small naming improvement
nalimilan Dec 4, 2016
0eab4dc
Review comments
nalimilan Dec 4, 2016
0a00622
Use generated function to avoid allocations on Julia 0.5
nalimilan Dec 6, 2016
615d084
Simplify code and improve performance on Julia 0.5, use only generate…
nalimilan Dec 7, 2016
75edfcb
Remove Julia 0.4 lines
nalimilan Dec 7, 2016
2d8670d
Update eltype computation code to match Base
nalimilan Jan 14, 2017
5f10b07
Fix deprecation warnings
nalimilan Jan 14, 2017
83ea4cf
Fix 0.6 deprecations
nalimilan Jan 14, 2017
3f17571
Require Compat 0.13.0
nalimilan Jan 19, 2017
5a88dab
Fix computing eltype
nalimilan Jan 19, 2017
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
Fix computing eltype
  • Loading branch information
@nalimilan
nalimilan committed Jan 19, 2017
commit 5a88dabb5186a87ca0d16257264ebfec04724b67
2 changes: 1 addition & 1 deletion src/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ if VERSION < v"0.6.0-dev" # Old approach needed for inference to work
@inline ziptype(A, B, C, D...) = Zip{Tuple{eltype(A)}, ziptype(B, C, D...)}
Copy link

Choose a reason for hiding this comment

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

Likewise here, the changes in JuliaLang/julia#19421 might be relevant. Best!


nullable_broadcast_eltype(f, As...) =
_default_eltype(Base.Generator{ziptype(As...), ftype(f, As...)})
eltype(_default_eltype(Base.Generator{ziptype(As...), ftype(f, As...)}))
Copy link

@pabloferz pabloferz Jan 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm curious why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

What is "this"?

Copy link

Choose a reason for hiding this comment

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

Sorry, the eltype you last added.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's useful because NullableArray{T} is parameterized on the eltype of Nullable rather than on the Nullable{T} itself.

Copy link

Choose a reason for hiding this comment

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

But, wouldn't that warrants that you don't get a Nullable{T} here?

Given that eltype(NullableArray([1], [true])) == Int, that implies _default_eltype(+, NullableArray([1], [true]), NullableArray([1], [true])) == Int, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, eltype(NullableArray([1], [true])) == Nullable{Int}.

I don't want T = Nullable{Int} here since it's used to build a new array via NullableArray{T}(...). NullableArray{Nullable{Int}} would have a Nullable{Nullable{Int}} eltype.

Copy link

Choose a reason for hiding this comment

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

The problem with this is that eltype(_default_eltype(*, ["a"], ["b"]) will give Char, so I guess you'd need a custom eltype here.

Copy link
Member Author

Choose a reason for hiding this comment

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

But that method is only called on NullableArrays, so we're certain the element types are Nullable (in your case, Nullable{String}).

Copy link

Choose a reason for hiding this comment

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

Oh, now I see, this is nullable_broadcast_eltype as opposed to broadcast_eltype. Sorry for the noise then.

else
Base.@pure nullable_eltypestuple(a) = Tuple{eltype(eltype(a))}
Base.@pure nullable_eltypestuple(T::Type) = Tuple{Type{eltype(T)}}
Expand Down