Skip to content
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

Merged
merged 7 commits into from
Jan 25, 2021

Conversation

davidwendt
Copy link
Contributor

This adds the libcudf part of #7157

std::unique_ptr<column> cudf::lists::count_elements(
  lists_column_view const& input,
  rmm::mr::device_memory_resource* mr);

Returns the size of each element in the input lists column.
The PR also includes gtests for this new API.

@davidwendt davidwendt added feature request New feature or request 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Jan 19, 2021
@davidwendt davidwendt self-assigned this Jan 19, 2021
@davidwendt davidwendt requested review from a team as code owners January 19, 2021 22:47
@davidwendt
Copy link
Contributor Author

rerun tests

@davidwendt davidwendt requested a review from a team as a code owner January 20, 2021 13:12
Copy link
Contributor

@nvdbaranec nvdbaranec left a 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
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #7173 (0a04bb0) into branch-0.18 (8860baf) will increase coverage by 0.08%.
The diff coverage is 86.25%.

Impacted file tree graph

@@               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     
Impacted Files Coverage Δ
python/cudf/cudf/_lib/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/core/column/lists.py 91.66% <ø> (-0.09%) ⬇️
python/cudf/cudf/utils/ioutils.py 78.71% <ø> (ø)
python/cudf/cudf/core/dataframe.py 90.71% <50.00%> (+<0.01%) ⬆️
python/cudf/cudf/io/orc.py 86.80% <60.00%> (-1.61%) ⬇️
python/cudf/cudf/core/dtypes.py 89.50% <66.66%> (-0.88%) ⬇️
python/cudf/cudf/core/series.py 91.10% <80.00%> (-0.06%) ⬇️
python/cudf/cudf/core/reshape.py 91.00% <81.81%> (-0.04%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.08% <83.33%> (-0.33%) ⬇️
python/cudf/cudf/io/csv.py 91.66% <86.66%> (-1.67%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a51caa5...0a04bb0. Read the comment docs.

Copy link
Contributor

@devavret devavret left a 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.

cpp/src/lists/count_elements.cu Outdated Show resolved Hide resolved
cpp/src/lists/count_elements.cu Show resolved Hide resolved
Copy link
Member

@harrism harrism left a 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.

@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6c2675c into rapidsai:branch-0.18 Jan 25, 2021
@davidwendt davidwendt deleted the fea-lists-count-elements branch January 30, 2021 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants