-
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
Refactor cython interface: copying.pyx
#10359
Refactor cython interface: copying.pyx
#10359
Conversation
…ent/ListOfColumnsRefactor/copying
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #10359 +/- ##
=================================================
+ Coverage 10.42% 86.15% +75.73%
=================================================
Files 119 139 +20
Lines 20603 22450 +1847
=================================================
+ Hits 2148 19342 +17194
+ Misses 18455 3108 -15347
Continue to review full report at Codecov.
|
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.
One important question about duplicating work and a couple of questions about establishing good patterns for APIs like this, but generally looks good.
cdef table_view target_table_view = table_view_from_columns( | ||
(target_column,)) | ||
cdef bool c_bounds_check = bounds_check | ||
cdef scatter_scalar(list source_device_slrs, |
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.
Something to think about: is there a better way for us to handle scalar vs column functions? We have this division in a lot of our Cython layers, can we think of a way to reduce it? One option would be to define generic scalar/column functions (not sure exactly what that would look like) and then passing specialized functions as arguments to handle different features. This is a very speculative idea, so no need to have an answer in this PR, but I do dislike duplicating this scalar/column division over and over.
…m:isVoid/cudf into improvement/ListOfColumnsRefactor/copying
…ent/ListOfColumnsRefactor/copying
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.
Some minor suggestions for improvement but it LGTM! I'll let you address the comments before merging.
…ent/ListOfColumnsRefactor/copying
Co-authored-by: Vyas Ramasubramani <[email protected]>
Co-authored-by: Vyas Ramasubramani <[email protected]>
…m:isVoid/cudf into improvement/ListOfColumnsRefactor/copying
@gpucibot merge |
Part of #10153
Aside from the two harder cases:
boolean_mask_scatter
andsample
that's been addressed in #10202 and #10262 , this PR tackles rest of refactors that's incopying.pyx
, in combination of the other two, this PR should address all interface refactor incopying.pyx
.