implemented NullableArray{T,N}(dims...) #152
Conversation
This is needed for instance when I have a NullableArray of some type and want to create another one of the same type with only nulls in it.
Current coverage is 85.89% (diff: 100%)@@ master #152 diff @@
==========================================
Files 14 14
Lines 862 865 +3
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 731 743 +12
+ Misses 131 122 -9
Partials 0 0
|
| (::Type{NullableArray{T,N}}){T,N}(dims::Vararg{Int,N}) = NullableArray(T, dims) | ||
| else | ||
| @compat (::Type{NullableArray{T,1}}){T,N}(x1) = NullableArray(T, x1) | ||
| @compat (::Type{NullableArray{T,2}}){T,N}(x1, x2) = NullableArray(T, x1, x2) |
There was a problem hiding this comment.
Why define these on 0.4 but not on 0.5? Performance?
There was a problem hiding this comment.
On 0.5 a single function takes care of all of them.
|
Is there any reason you can't use julia> A = collect(1:4)
4-element Array{Int64,1}:
1
2
3
4
julia> X = NullableArray(A)
4-element NullableArrays.NullableArray{Int64,1}:
1
2
3
4
julia> Y = similar(X)
4-element NullableArrays.NullableArray{Int64,1}:
#NULL
#NULL
#NULL
#NULL |
|
@davidagold |
|
I don't need this anymore. |
|
I think it would still be good to have. |
|
@nalimilan ok, I fixed the errors on v"0.4" |
| @@ -34,6 +34,14 @@ end | |||
|
|
|||
| @compat (::Type{NullableArray{T}}){T}(dims::Dims) = NullableArray(T, dims) | |||
| @compat (::Type{NullableArray{T}}){T}(dims::Int...) = NullableArray(T, dims) | |||
There was a problem hiding this comment.
Is this definition still useful?
There was a problem hiding this comment.
It's needed in order to do things like NullableArray{Int}(3,2).
But a call to NullableArray{T}(dims::Int...) such as A(dim1,dim2) can always be replaced by A((dim1,dim2)) to call the other constructor with ::Dims instead. And A(dims...) can be replaced with A((dims...)). I think it is convenient keep this though.
There was a problem hiding this comment.
Ah, right, I thought N could be inferred automatically in your new constructor, but that doesn't work for the call overloading syntax.
|
@davidagold Good to merge? |
| function Base.convert{T,N}(::Type{NullableArray{T,N}}, dims::Int...) | ||
| length(dims) == N || throw(ArgumentError("Wrong number of arguments. Expected $N, got $(length(dims)).")) | ||
| NullableArray(T, dims) | ||
| end |
There was a problem hiding this comment.
Please use 4 space indents for consistency with the rest of the repo.
|
What's the failure? |
|
Changed my review since my comments were addressed, but I think the cause of the test failure should be determined before this is merged. |
|
I don't see how the error is related to this PR. v0.4 and v0.5 pass. but not nightly. does the tests currently pass on master? |
|
The error is unrelated, I'm able to reproduce it on master too. Trying to figure out what's going on... EDIT: that's a bug in Base, see JuliaLang/julia#19270. |
|
All is green now, merging. |
|
Damn, I screwed up and forgot to squash. Squashed and force-pushed manually. |
This is needed for instance when I have a NullableArray
Aof some type and want to create another one of the same type with only nulls in it like so:typeof(A)(size(A)...)