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

Use device_uvector, device_span in sort groupby #7523

Merged
merged 6 commits into from
Mar 15, 2021
Merged

Use device_uvector, device_span in sort groupby #7523

merged 6 commits into from
Mar 15, 2021

Conversation

karthikeyann
Copy link
Contributor

  • Replace device_vector with device_uvector
  • Replace device_vector const& with device_span

Ref. #7387 (comment)

@karthikeyann karthikeyann added 3 - Ready for Review Ready for review by team tech debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 5, 2021
@karthikeyann karthikeyann requested a review from a team as a code owner March 5, 2021 22:12
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 5, 2021
@karthikeyann karthikeyann mentioned this pull request Mar 5, 2021
Merged
@karthikeyann
Copy link
Contributor Author

rerun tests

@jrhemstad
Copy link
Contributor

CI is failing due to a segfault in a groupby test. You should verify that none of the places where device_vector was replaced with device_uvector relied on the default initialization of device_vector.

hyperbolic2346
hyperbolic2346 approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@hyperbolic2346 hyperbolic2346 left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@codecov
Copy link

codecov bot commented Mar 9, 2021 •
edited
Loading

Codecov Report

Merging #7523 (aff7e72) into branch-0.19 (53929eb) will increase coverage by 0.51%.
The diff coverage is 92.80%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7523      +/-   ##
===============================================
+ Coverage        81.88%   82.40%   +0.51%     
===============================================
  Files              101      101              
  Lines            16900    17342     +442     
===============================================
+ Hits             13838    14290     +452     
+ Misses            3062     3052      -10
Impacted Files Coverage Δ
python/cudf/cudf/core/index.py 93.34% <ø> (+0.48%) ⬆️
python/cudf/cudf/core/column/decimal.py 93.33% <84.61%> (-2.50%) ⬇️
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/dataframe.py 90.58% <95.00%> (+0.11%) ⬆️
python/cudf/cudf/core/series.py 91.57% <95.55%> (+0.78%) ⬆️
python/cudf/cudf/core/indexing.py 96.29% <100.00%> (+0.23%) ⬆️
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <0.00%> (ø)
... and 45 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 53929eb...016bcef. Read the comment docs.

@jrhemstad
Copy link
Contributor

rerun tests

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. Some of these files need their copyright year updated.

@karthikeyann karthikeyann added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Mar 15, 2021
@karthikeyann
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 325d5b8 into rapidsai:branch-0.19 Mar 15, 2021
@karthikeyann karthikeyann mentioned this pull request Mar 16, 2021
Closed
26 tasks
@mythrocks mythrocks mentioned this pull request Mar 17, 2021
Merged
rapids-bot bot pushed a commit that referenced this pull request Mar 21, 2021
@mythrocks
Fix Debug build break with device_uvectors in grouped_rolling.cu (#7633)
11455ee 
After #7523, on `Debug` builds, one seems to see the following build error:
```
cudf/cpp/src/rolling/grouped_rolling.cu(130): error: no operator "[]" matches these operands
            operand types are: const cudf::groupby::detail::sort::sort_groupby_helper::index_vector [ int ]
...
cudf/cpp/src/rolling/grouped_rolling.cu(130): error: no operator "[]" matches these operands
            operand types are: const cudf::groupby::detail::sort::sort_groupby_helper::index_vector [ unsigned long ] 
```

The offending line has an `assert()` that is only compiled during `Debug` builds. This commit corrects the indexing into `device_uvector`.

Authors:
  - MithunR (@mythrocks)

Approvers:
  - David (@davidwendt)
  - Nghia Truong (@ttnghia)
  - Devavret Makkar (@devavret)
  - Karthikeyan (@karthikeyann)

URL: #7633
hyperbolic2346 pushed a commit to hyperbolic2346/cudf that referenced this pull request Mar 25, 2021
@karthikeyann @hyperbolic2346
Use device_uvector, device_span in sort groupby (rapidsai#7523)
a7ff744 
- Replace device_vector with device_uvector
- Replace device_vector const& with device_span<const>

Ref. rapidsai#7387 (comment)

Authors:
  - Karthikeyan (@karthikeyann)

Approvers:
  - Mike Wilson (@hyperbolic2346)
  - David (@davidwendt)

URL: rapidsai#7523
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

@hyperbolic2346 hyperbolic2346 hyperbolic2346 approved these changes

@trxcllnt trxcllnt Awaiting requested review from trxcllnt

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function 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.
4 participants
@karthikeyann @jrhemstad @hyperbolic2346 @davidwendt