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

address vcat return inconsistency #187

Closed
wants to merge 13 commits into from
77 changes: 77 additions & 0 deletions src/nullablevector.jl
Original file line number Diff line number Diff line change
Expand Up @@ -311,3 +311,80 @@ function Base.empty!(X::NullableVector)
empty!(X.isnull)
return X
end

function Base.promote_rule{T1,T2}(::Type{T1}, ::Type{Nullable{T2}})
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually needed (and would have to live in Base anyway). Indeed you should never call promote_rule directly, but use promote_type instead. This already works:

julia> promote_type(Nullable{Int64}, Int64)
Nullable{Int64}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I see now that promote_type has the consistency I was expecting from promote_rule. I'll need to learn more about those differences.

julia> promote_rule(Nullable{Int64}, Int64)
Nullable{Int64}

julia> promote_rule(Int64, Nullable{Int64})
Union{}

julia> promote_type(Nullable{Int64}, Int64)
Nullable{Int64}

julia> promote_type(Int64, Nullable{Int64})
Nullable{Int64}

promote_rule(Nullable{T2}, T1)
end

function Base.typed_hcat{T}(::Type{T}, A::AbstractVecOrMat...)
Copy link
Member

Choose a reason for hiding this comment

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

It's not OK to override Base methods like this with signatures that only involve Base types. That will affect unrelated packages and is known as type piracy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes perfect sense. Thanks for explaining what type piracy is

nargs = length(A)
nrows = size(A[1], 1)
ncols = 0
dense = true
for j = 1:nargs
Aj = A[j]
if size(Aj, 1) != nrows
throw(ArgumentError("number of rows of each array must match (got $(map(x->size(x,1), A)))"))
end
dense &= isa(Aj,Array)
nd = ndims(Aj)
ncols += (nd==2 ? size(Aj,2) : 1)
end
i = findfirst(a -> isa(a, NullableArray), A)
B = similar(full(A[i == 0 ? 1 : i]), T, nrows, ncols)
pos = 1
if dense
for k=1:nargs
Ak = A[k]
n = length(Ak)
copy!(B, pos, Ak, 1, n)
pos += n
end
else
for k=1:nargs
Ak = A[k]
p1 = pos+(isa(Ak,AbstractMatrix) ? size(Ak, 2) : 1)-1
B[:, pos:p1] = Ak
pos = p1+1
end
end
return B
end

function Base.typed_vcat{T}(::Type{T}, V::AbstractVector...)
n::Int = 0
for Vk in V
n += length(Vk)
end
i = findfirst(v -> isa(v, NullableArray), V)
a = similar(full(V[i == 0 ? 1 : i]), T, n)
pos = 1
for k=1:length(V)
Vk = V[k]
p1 = pos+length(Vk)-1
a[pos:p1] = Vk
pos = p1+1
end
a
end

function Base.typed_vcat{T}(::Type{T}, A::AbstractMatrix...)
nargs = length(A)
nrows = sum(a->size(a, 1), A)::Int
ncols = size(A[1], 2)
for j = 2:nargs
if size(A[j], 2) != ncols
throw(ArgumentError("number of columns of each array must match (got $(map(x->size(x,2), A)))"))
end
end
i = findfirst(a -> isa(a, NullableArray), A)
B = similar(full(A[i == 0 ? 1 : i]), T, nrows, ncols)
pos = 1
for k=1:nargs
Ak = A[k]
p1 = pos+size(Ak,1)-1
B[pos:p1, :] = Ak
pos = p1+1
end
return B
end
9 changes: 8 additions & 1 deletion src/operators.jl
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
## Lifted operators

importall Base.Operators
import Base: promote_op, abs, abs2, sqrt, cbrt, scalarmin, scalarmax, isless
import Base: abs,
abs2,
cbrt,
isless,
promote_op,
scalarmin,
scalarmax,
sqrt
using Compat: @compat, @functorize

if isdefined(Base, :fieldname) && Base.fieldname(Nullable, 1) == :hasvalue # Julia 0.6
Expand Down
8 changes: 8 additions & 0 deletions test/nullablevector.jl
Original file line number Diff line number Diff line change
Expand Up @@ -223,4 +223,12 @@ module TestNullableVector
X = NullableArray(A, M)
empty!(X)
@test isempty(X)

# tests for reciprocal behavior
@test promote_rule(Nullable{Int64}, Int64) == Nullable{Int64}
@test promote_rule(Int64, Nullable{Int64}) == Nullable{Int64}
@test typeof(hcat(NullableArray(1:2), 1:2)) == NullableArrays.NullableArray{Int64,2}
@test typeof(hcat(1:2, NullableArray(1:2))) == NullableArrays.NullableArray{Int64,2}
@test typeof(vcat(NullableArray(1:2), 1:2)) == NullableArrays.NullableArray{Int64,1}
@test typeof(vcat(1:2, NullableArray(1:2))) == NullableArrays.NullableArray{Int64,1}
end