-
Notifications
You must be signed in to change notification settings - Fork 21
Fix issue #100, make work again with v0.5 #101
Conversation
@andreasnoack @ElOceanografo Could somebody from JuliaStats review this and possibly merge this? |
const unsafe_getindex = Base.unsafe_getindex | ||
|
||
function Base.isnull{T,N,P<:NullableArray,IV,LD}(V::SubArray{T,N,P,IV,LD}, I::Int...) | ||
@Base._inline_meta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use @inline function...
. This private macro only exists for bootstrapping.
There will be a performance regression for linear indexing with linear fast SubArrays here. That can be avoided with an additional method that uses the stride computation as in https://github.com/JuliaLang/julia/blob/57c53afba29a06493a2d993fb571a992d0ad733a/base/subarray.jl#L151. |
function Base.isnull{T,N,P<:NullableArray,IV,LD}(V::SubArray{T,N,P,IV,LD}, I::Int...) | ||
@Base._inline_meta | ||
@boundscheck checkbounds(V, I...) | ||
@inbounds r = V.parent.isnull[Base.reindex(V, V.indexes, Base.to_indexes(I...))...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use to_indexes
. They're already all Int.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out, to_indexes for some reason is necessary, if I don't use it, it gets an error, saying that it can't find a method of reindex that matches. Any idea why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you splatting I
? Don't splat I
.
Could you move the unrelated indentation changes to a separate commit? |
@@ -0,0 +1,25 @@ | |||
const unsafe_getindex = Base.unsafe_getindex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to be used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was just in the original code, so I left it in, didn't know if it was necessary. I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out it is necessary for the v0.4 code.
fedd5cb
to
144e57c
Compare
@mbauman @nalimilan I think this is all set to go now. Thanks to both of you for the review (learned some neat stuff about the new subarray changes to boot!) |
|
||
@inline function Base.isnull(V::NullableSubArray, I::Real...) | ||
@boundscheck checkbounds(V, I...) | ||
@inbounds return V.parent.isnull[Base.reindex(V, V.indexes, Base.to_indexes(I...))...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the signature the same between 0.4 and 0.5, and don't use to_indexes
. Widening the signature is an unrelated change from what's required here, and it introduces a feature difference between code that's meant to be functionally the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't work without to_indexes
. I removed them, and then got an method error about reindex
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What did you try, and what error did you get? Don't splat I
. Just V.parent.isnull[Base.reindex(V, V.indexes, I)...]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind - the problem was an issue with splatting, I needed to remove both the to_index call and the splatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you introduce these to_index
calls in one commit, and remove them in the next one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nalimilan They were necessary to have the subscripts take Real, which would be more consistent with the code in Base/subarray.jl, however, I removed them based on @mbauman review comment above.
I do feel that it's incorrect to have these act differently than plain SubArrays, however, that could be fixed in a separate PR if people agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably the commits will be squashed, removing the the addiction and
subtraction of that feature? I believe this is what @naliman was referring
to.
On Monday, April 4, 2016, Scott P. Jones notifications@github.com wrote:
In src/subarraynew.jl
#101 (comment)
:@@ -0,0 +1,36 @@
+typealias NullableSubArray{T,N,P<:NullableArray,IV,LD} SubArray{T,N,P,IV,LD}
+
+@inline function Base.isnull(V::NullableSubArray, I::Real...)
- @BoundsCheck checkbounds(V, I...)
- @inbounds return V.parent.isnull[Base.reindex(V, V.indexes, Base.to_indexes(I...))...]
@nalimilan https://github.com/nalimilan They were necessary to have the
subscripts take Real, which would be more consistent with the code in
Base/subarray.jl, however, I removed them based on @mbauman
https://github.com/mbauman review comment above.
I do feel that it's incorrect to have these act differently than plain
SubArrays, however, that could be fixed in a separate PR if people agree.—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
https://github.com/JuliaStats/NullableArrays.jl/pull/101/files/a2a4740c12a904f5bd11672d1190ef0eca59b02f..6a849974f7af5b75df58727dbe9f22dc5b978960#r58371197
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kmsquire I was asked to keep the fixing of the indentation as a separate commit, and I think the fixes related to adding Compat should probably be in a separate commit from the fixes related to JuliaLang/julia#15071, but I'll squash out that commit. Does that sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect
On Monday, April 4, 2016, Scott P. Jones notifications@github.com wrote:
In src/subarraynew.jl
#101 (comment)
:@@ -0,0 +1,36 @@
+typealias NullableSubArray{T,N,P<:NullableArray,IV,LD} SubArray{T,N,P,IV,LD}
+
+@inline function Base.isnull(V::NullableSubArray, I::Real...)
- @BoundsCheck checkbounds(V, I...)
- @inbounds return V.parent.isnull[Base.reindex(V, V.indexes, Base.to_indexes(I...))...]
@kmsquire https://github.com/kmsquire I was asked to keep the fixing of
the indentation as a separate commit, and I think the fixes related to
adding Compat should probably be in a separate commit from the fixes
related to JuliaLang/julia#15071
JuliaLang/julia#15071, but I'll squash out that
commit. Does that sound reasonable?—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/JuliaStats/NullableArrays.jl/pull/101/files/a2a4740c12a904f5bd11672d1190ef0eca59b02f..6a849974f7af5b75df58727dbe9f22dc5b978960#r58378574
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was what I meant.
Besides #101 (comment) the SubArray commit looks good to me. I didn't review the other commits. |
OK, done now, fixed all of the subarray issues, the other issue was simply using |
LGTM. Thanks @ScottPJones. |
Bump! Anybody can merge this fix around? Thanks! |
@ScottPJones Thank you very much for this =) I have just one request -- could you please change the filename |
Remove to_indexes/to_index, Int instead of Real indices Rename subarraynew.jl subarray0.5.jl
@davidagold Very reasonable! (I should have thought of that myself! 😳) Thank you for the nice package! |
This fixes deprecations due to removal of
call
overloading,and also fixes a problem with handling subarrays,
due to JuliaLang/julia#15071 getting rid of the unexported
index_generate
function.