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.

Sign up for GitHub

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

Jump to bottom

Fix cudf::lists::sort_lists failing for sliced column #7564

Merged
merged 8 commits into from
Mar 11, 2021
Merged

Fix cudf::lists::sort_lists failing for sliced column #7564

merged 8 commits into from
Mar 11, 2021

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Mar 10, 2021 •
edited
Loading

This fixes #7530 (cudf::lists::sort_lists fails for sliced column).

I also added more tests for sliced columns to cover the previously failed cases, and added a header lists/detail/sorting.cuh to expose the internal detail::sort_lists API which accepts a stream parameter.

@ttnghia ttnghia requested a review from a team as a code owner March 10, 2021 23:48
@ttnghia ttnghia requested review from trxcllnt and jrhemstad March 10, 2021 23:48
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 10, 2021
@ttnghia ttnghia added bug Something isn't working non-breaking Non-breaking change labels Mar 10, 2021
@ttnghia ttnghia changed the title Fix sort list Fix cudf::lists::sort_lists failing for sliced column Mar 10, 2021
@codecov
Copy link

codecov bot commented Mar 11, 2021 •
edited
Loading

Codecov Report

Merging #7564 (2219774) into branch-0.19 (7871e7a) will increase coverage by 0.51%.
The diff coverage is 92.85%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7564      +/-   ##
===============================================
+ Coverage        81.86%   82.38%   +0.51%     
===============================================
  Files              101      101              
  Lines            16884    17339     +455     
===============================================
+ Hits             13822    14284     +462     
+ Misses            3062     3055       -7
Impacted Files Coverage Δ
python/cudf/cudf/core/index.py 93.34% <ø> (+0.48%) ⬆️
python/cudf/cudf/core/column/column.py 87.80% <75.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/numerical.py 94.85% <85.71%> (-0.17%) ⬇️
python/cudf/cudf/core/frame.py 89.12% <89.47%> (+0.10%) ⬆️
python/cudf/cudf/core/column/decimal.py 93.33% <90.47%> (-1.54%) ⬇️
python/cudf/cudf/core/dataframe.py 90.58% <95.00%> (+0.11%) ⬆️
python/cudf/cudf/core/series.py 91.57% <95.55%> (+0.78%) ⬆️
python/cudf/cudf/core/column/string.py 86.76% <100.00%> (+0.26%) ⬆️
python/cudf/cudf/core/indexing.py 96.29% <100.00%> (+0.23%) ⬆️
python/cudf/cudf/utils/gpu_utils.py 53.65% <0.00%> (-4.88%) ⬇️
... and 50 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 35f3f70...b57702f. Read the comment docs.

karthikeyann
karthikeyann requested changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpp/include/cudf/lists/detail/sorting.cuh need not be cuh.
it should be .hpp

cpp/include/cudf/lists/detail/sorting.cuh Outdated Show resolved Hide resolved
@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 11, 2021

Thanks for your recommendation. I have changed lists/detail/sorting.cuh to lists/detail/sorting.hpp.

@ttnghia ttnghia requested a review from a team as a code owner March 11, 2021 13:46
@github-actions github-actions bot added the conda label Mar 11, 2021
davidwendt
davidwendt approved these changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@davidwendt davidwendt 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.

karthikeyann
karthikeyann approved these changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 11, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit eaec3db into rapidsai:branch-0.19 Mar 11, 2021
hyperbolic2346 pushed a commit to hyperbolic2346/cudf that referenced this pull request Mar 25, 2021
@ttnghia @hyperbolic2346
Fix cudf::lists::sort_lists failing for sliced column (rapidsai#7564)
b7dd2cd 
This fixes rapidsai#7530 (`cudf::lists::sort_lists` fails for sliced column). 

I also added more tests for sliced columns to cover the previously failed cases, and added a header `lists/detail/sorting.cuh` to expose the internal `detail::sort_lists` API which accepts a stream parameter.

Authors:
  - Nghia Truong (@ttnghia)

Approvers:
  - David (@davidwendt)
  - AJ Schmidt (@ajschmidt8)
  - Karthikeyan (@karthikeyann)

URL: rapidsai#7564
@ttnghia ttnghia self-assigned this Apr 25, 2021
@ttnghia ttnghia deleted the fix_sort_list branch May 3, 2021 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

@trxcllnt trxcllnt Awaiting requested review from trxcllnt

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

Assignees

@ttnghia ttnghia

Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] cudf::lists::sort_lists fails for sliced column
4 participants
@ttnghia @karthikeyann @ajschmidt8 @davidwendt