-
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
Rewrite select!
to improve performance
#1985
Conversation
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.
Looks good. Now when I look at it it can be even made shorter by using copy!
and using the pattern discussed by @nalimilan in #1974.
So it would look like:
copy!(_columns(df), _columns(df)[inds])
x = index(df)
copy!(_names(x), _names(df)[inds])
for (i, n) in enumerate(x.names)
x.lookup[n] = i
end
select!
to improve performanceselect!
to improve performance[WIP]
Need to import Future.copy! for backwards compatibility with ver 1.0. Will do it tomorrow. |
Yes - exactly like in #1974 (so whichever gets merged first the other will need to rebase later). |
Can you tell me why something like |
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 think it is OK.
@ararslan - just please confirm if this conditional import is OK
I think we are ready to merge. |
select!
to improve performance[WIP]select!
to improve performance
I am OK to merge this. Let us wait for @nalimilan or @ararslan to confirm so that we have a second pair of eyes :), as this is a fundamental function in the DataFrames.jl ecosystem 😄. |
@Ellipse0934 and @nalimilan - thank you for contributing to this. The change is really of order of magnitude improvement as @Ellipse0934 noted! |
As discussed in Issue #1861 . This will significantly improve performance for the
select!
function(over 1000x for 100,000 columns).Passes the tests locally and I couldn't find any bugs upon testing.