-
Notifications
You must be signed in to change notification settings - Fork 908
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
Add libcudf lists column count_elements API #7173
Add libcudf lists column count_elements API #7173
Conversation
rerun tests |
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 to me. Couldn't hurt to add some tests that include null elements in the individual lists, even though the code isn't really affected by them
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #7173 +/- ##
===============================================
+ Coverage 82.09% 82.17% +0.08%
===============================================
Files 97 97
Lines 16474 16543 +69
===============================================
+ Hits 13524 13594 +70
+ Misses 2950 2949 -1
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.
It occurred to me that list related structures are a bit of a mess. list_column_device_view
's ctor is only host callable. There's an empty list_view
class, yet there's a separate list_device_view
class. And that despite there being a class that describes one list element, there's no element<>()
accessor for it.
All of this is out of scope of this PR but just adding the latter would make this a bit proper.
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.
cmake approval. Won't merge because I see there is still some discussion. @davidwendt merge when ready.
@gpucibot merge |
This adds the libcudf part of #7157
Returns the size of each element in the input lists column.
The PR also includes gtests for this new API.