-
Notifications
You must be signed in to change notification settings - Fork 915
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
Use list of column inputs for apply_boolean_mask
#9832
Use list of column inputs for apply_boolean_mask
#9832
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-22.02 #9832 +/- ##
================================================
- Coverage 10.49% 10.41% -0.08%
================================================
Files 119 119
Lines 20305 20484 +179
================================================
+ Hits 2130 2134 +4
- Misses 18175 18350 +175
Continue to review full report at Codecov.
|
…ent/use_list_of_columns_apply_boolean_mask
…ent/use_list_of_columns_apply_boolean_mask
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 don't have much to add here, the code looks good. However, the methods in BaseIndex and IndexedFrame are identical aside from the passing of the index columns to the _from_columns
, so while I think we can move ahead with this for now we should keep thinking about how to abstract away the column selection to pass the Cython APIs. Once all Cython APIs are transitioned to the list of columns approach I think that's the next frontier in simplifying these code paths. We've discussed it before but not come up with a satisfactory resolution IIRC.
…ent/use_list_of_columns_apply_boolean_mask
…ent/use_list_of_columns_apply_boolean_mask
@gpucibot merge |
This PR brings changes from #9558 to
apply_boolean_mask
and removes theas_frame
->as_column
round trip. Benchmark the column method:Dataframe benchmark