Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

Fix broken getindex methods, clean up the rest. Speed up find(*DataVector{Bool}). #24

Merged
merged 1 commit into from
Dec 3, 2013

Conversation

garborg
Copy link
Member

@garborg garborg commented Dec 2, 2013

I noticed this because it was breaking subsetting of DataFrames by expression.

end
function Base.getindex(x::Vector,
inds::AbstractDataArray{Bool})
return x[find(array(inds, replace = false))]
return x[find(array(inds, false))]
Copy link
Member

Choose a reason for hiding this comment

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

I think this can just be find(inds) like the others.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't see an appropriate find method:

da = @data [true false; NA true]
find(array(da, false)) # is fine
find(da) # find(A::AbstractArray{T,N}) at array.jl:1231 can't handle it
find(da[:, 1]) # find(dv::AbstractDataArray{Bool,1}) at datavector.jl:17 handles it

Copy link
Member

Choose a reason for hiding this comment

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

You're right. Maybe we should fix the method signature for find instead of working around that here, since find in Base is happy to take n-dimensional arrays.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense -- happy to take a crack in the evening.

@johnmyleswhite
Copy link
Member

I'm a little confused by the mixture of calls to a version of find that can't handle NA and calls to a version that can. Is that a reflection of this being a work-in-progress?

@garborg
Copy link
Member Author

garborg commented Dec 3, 2013

I was basically just taking care of typos and and calls to deprecated functions without getting into design, but I cleared it up now.

@johnmyleswhite
Copy link
Member

This is good to go. Could you squash these commits for a clean merge?

garborg added a commit that referenced this pull request Dec 3, 2013
Clean up getindex and find, remove calls to replaceNA
@garborg garborg merged commit b4b15c6 into JuliaStats:master Dec 3, 2013
@johnmyleswhite
Copy link
Member

Thank you!

@garborg
Copy link
Member Author

garborg commented Dec 3, 2013

No problem, I appreciate the direction.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants