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.
This is another possible ref into an array with #undefs. Maybe we need a function that just checks for that.
There was a problem hiding this comment.
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.
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.
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.
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
getindexandsetindex!. It would be great if someone could try it out and confirm that it works.