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
Closed

address vcat return inconsistency #187

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1689728
address https://github.com/JuliaStats/NullableArrays.jl/issues/167
cjprybol Feb 24, 2017
a36f6a4
reorder import statements
cjprybol Feb 24, 2017
5d9d982
move from operators files to nullablevector files
cjprybol Feb 24, 2017
4aa635c
reduce the piracy
cjprybol Feb 25, 2017
7d948a0
no more piracy!
cjprybol Feb 25, 2017
1b64de2
passing tests
cjprybol Feb 25, 2017
9cc8381
rearrange functions to cleanup comments
cjprybol Feb 25, 2017
b8c87b5
spacing
cjprybol Feb 25, 2017
bb0b1ee
add more tests and reorder tests to match function order
cjprybol Feb 25, 2017
b8211a9
no more stack overflows
cjprybol Feb 25, 2017
d28b7b8
Int64 -> Int
cjprybol Feb 25, 2017
6c30d14
implement 2 argument cases and comment out 3 argument tests
cjprybol Mar 1, 2017
2e2b98a
correct spacing issue
cjprybol Mar 2, 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
26 changes: 26 additions & 0 deletions src/nullablevector.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import Base: vcat, hcat, typed_vcat, typed_hcat, promote_eltype

"""
push!{T, V}(X::NullableVector{T}, v::V)

Expand Down Expand Up @@ -311,3 +313,27 @@ function Base.empty!(X::NullableVector)
empty!(X.isnull)
return X
end

function vcat(A::AbstractVector, B::AbstractVector...)
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 still type piracy. There needs to be NullableArray in the signature. Currently I don't think this can be fixed in full generality. We can include a few methods for all possible combinations up to e.g. 3 arguments, i.e:

  • ::NullableArray, ::AbstractArray
  • ::AbstractArray, ::NullableArray
  • ::NullableArray, ::AbstractArray, ::AbstractArray
  • ::AbstractArray, ::NullableArray, ::AbstractArray
  • ::AbstractArray, ::AbstractArray, ::NullableArray

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 think defining these functions would be a regression compared to just leaving the current functionality as is.

# this rule seems more intuitive
isa(array[1], NullableArray) ? NullableArray{T} : Array[Nullable{T}]

# than this rule
isa(array[1], NullableArray) ||
isa(array[2], NullableArray) ||
isa(array[3], NullableArray) ? NullableArray{T} : Array[Nullable{T}]

If the general functionality is type piracy then I feel the best way forward would be to either address JuliaLang/julia#2326 directly or accept the current behavior. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I really don't know. Clearly we can't reach a satisfying state without improvements in Julia. But which of the partial solutions is better, I don't know...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar sentiments here. I'll implement the 2 argument cases for hcat/vcat and now at least we have everything written up and documented.

Copy link
Member

Choose a reason for hiding this comment

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

If you implement the two-argument versions, please also implement the three-argument versions. Else the rule is going to be even more complex than what you wrote above.

Of course, this rests on the assumption that concatenating more than 3 arrays is a somewhat rare operation, so that it's not a big issue if other variants behave differently. Might not be the case, but only special-casing two-argument methods is clearly a worse solution.

any(b -> isa(b, NullableArray), B) ?
typed_vcat(promote_eltype(A, B...), NullableArray(A), B...) :
typed_vcat(promote_eltype(A, B...), A, B...)
end

function vcat(A::AbstractMatrix, B::AbstractMatrix...)
any(b -> isa(b, NullableArray), B) ?
typed_vcat(promote_eltype(A, B...), NullableArray(A), B...) :
typed_vcat(promote_eltype(A, B...), A, B...)
end

function vcat(A::AbstractArray, B::AbstractArray...)
any(b -> isa(b, NullableArray), B) ?
typed_vcat(promote_eltype(A, B...), NullableArray(A), B...) :
typed_vcat(promote_eltype(A, B...), A, B...)
end

function hcat(A::AbstractVecOrMat, B::AbstractVecOrMat...)
any(b -> isa(b, NullableArray), B) ?
typed_hcat(promote_eltype(A, B...), NullableArray(A), B...) :
typed_hcat(promote_eltype(A, B...), A, B...)
end
22 changes: 22 additions & 0 deletions test/nullablevector.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,4 +223,26 @@ module TestNullableVector
X = NullableArray(A, M)
empty!(X)
@test isempty(X)

@test typeof(vcat(NullableArray(1:2), 3:4)) == NullableArray{Int,1}
@test typeof(vcat(1:2, NullableArray(3:4))) == NullableArray{Int,1}
@test typeof(vcat(NullableArray([1 2]), [3 4])) == NullableArray{Int,2}
@test typeof(vcat([1 2], NullableArray([3 4]))) == NullableArray{Int,2}
@test typeof(vcat(NullableArray(1:2), 3:4, 5:6)) == NullableArray{Int,1}
@test typeof(vcat(1:2, NullableArray(3:4), 5:6)) == NullableArray{Int,1}
@test typeof(vcat(1:2, 3:4, NullableArray(5:6))) == NullableArray{Int,1}
@test typeof(vcat(NullableArray([1 2]), [3 4], [5 6])) == NullableArray{Int,2}
@test typeof(vcat([1 2], NullableArray([3 4]), [5 6])) == NullableArray{Int,2}
@test typeof(vcat([1 2], [3 4], NullableArray([5 6]))) == NullableArray{Int,2}

@test typeof(hcat(NullableArray(1:2), 3:4)) == NullableArray{Int,2}
@test typeof(hcat(1:2, NullableArray(3:4))) == NullableArray{Int,2}
@test typeof(hcat(NullableArray([1 2]), [3 4])) == NullableArray{Int,2}
@test typeof(hcat([1 2], NullableArray([3 4]))) == NullableArray{Int,2}
@test typeof(hcat(NullableArray(1:2), 3:4, 5:6)) == NullableArray{Int,2}
@test typeof(hcat(1:2, NullableArray(3:4), 5:6)) == NullableArray{Int,2}
@test typeof(hcat(1:2, 3:4, NullableArray(5:6))) == NullableArray{Int,2}
@test typeof(hcat(NullableArray([1 2]), [3 4], [5 6])) == NullableArray{Int,2}
@test typeof(hcat([1 2], NullableArray([3 4]), [5 6])) == NullableArray{Int,2}
@test typeof(hcat([1 2], [3 4], NullableArray([5 6]))) == NullableArray{Int,2}
end