-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to get/set methods #213
Conversation
@johnmyleswhite you seem to missing several ref/assigns. Search for |
Thanks! I'll finish this today. |
Conflicts: src/dataarray.jl Many other fixes: update for cor(), svd(), etc.
I think this is ready to go. I've also made a lot of other fixes that we needed. @kmsquire, note that the two calls to |
I'll merge this tomorrow morning barring any objections. |
# TODO: Make inds::AbstractVector | ||
## # There are two definitions in order to remove ambiguity warnings | ||
getindex{T<:Number,N}(d::DataArray{T,N}, inds::Union(BitVector, Vector{Bool})) = DataArray(d.data[inds], d.na[inds]) | ||
getindex{T<:Number,N}(d::DataArray{T,N}, inds::Union(Vector, Ranges, BitVector)) = DataArray(d.data[inds], d.na[inds]) |
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 is another possible ref into an array with #undef
s. Maybe we need a function that just checks for that.
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.
I still need to ponder this more. Why is indexing an undef
so bad? It's possible in Base Julia, although it throws an error:
julia> a = Array(ASCIIString, 5)
5-element ASCIIString Array:
#undef
#undef
#undef
#undef
#undef
julia> a[1]
ERROR: access to undefined reference
in getindex at array.jl:246
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 is issue #198. If you end up with undefs in d.data, but d.na is all true's, indexing into that should return NA. That's the DataArray you get from using a = DataArray(Array(ASCIIString, 5))
. We need one of two rules:
- If at all possible, don't allow undefs in d.data.
- Before indexing into d.data, check d.na to see if there are NAs.
As part of a commit to start to address #198, I used option 2, but option 1 may be the better approach.
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.
Oh, I very strongly think that Option 2 is the way to go. Option 1 is like building a dam against the ocean when dealing with strings.
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.
Can we build up a good test suite for this? I have some examples in mind, but it would be good to pinpoint all the places where NA's are not being returned because of undef's.
Thanks for doing this, John. No objections here. |
Okay, thanks. Maybe it would be good to add a comment to that effect? |
Done. |
Switch to get/set methods
Merged. |
This pull request tries to make the full transition over to
getindex
andsetindex!
. It would be great if someone could try it out and confirm that it works.