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

Fix Debug build break with device_uvectors in grouped_rolling.cu #7633

Merged
merged 6 commits into from
Mar 21, 2021

Conversation

mythrocks
Copy link
Contributor

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.

@mythrocks mythrocks requested a review from a team as a code owner March 17, 2021 23:27
@mythrocks mythrocks self-assigned this Mar 17, 2021
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 17, 2021
@mythrocks mythrocks added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond bug Something isn't working non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. labels Mar 17, 2021
@mythrocks mythrocks changed the title Fix indexing into device_uvector Fix indexing into device_uvector in grouped_rolling.cu Mar 17, 2021
@mythrocks
Copy link
Contributor Author

Alternatively, we might just remove the assert().

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 18, 2021
@mythrocks mythrocks changed the title Fix indexing into device_uvector in grouped_rolling.cu Fix Debug build break with device_uvectors in grouped_rolling.cu Mar 18, 2021
@codecov
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #7633 (d5c8212) into branch-0.19 (7871e7a) will increase coverage by 0.60%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7633      +/-   ##
===============================================
+ Coverage        81.86%   82.47%   +0.60%     
===============================================
  Files              101      101              
  Lines            16884    17397     +513     
===============================================
+ Hits             13822    14348     +526     
+ Misses            3062     3049      -13     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/categorical.py 91.97% <ø> (+0.58%) ⬆️
python/cudf/cudf/core/column/column.py 87.86% <ø> (+0.10%) ⬆️
python/cudf/cudf/core/column/datetime.py 89.63% <ø> (+0.54%) ⬆️
python/cudf/cudf/core/column/decimal.py 92.75% <ø> (-2.12%) ⬇️
python/cudf/cudf/core/column/lists.py 92.17% <ø> (+0.77%) ⬆️
python/cudf/cudf/core/column/numerical.py 94.83% <ø> (-0.20%) ⬇️
python/cudf/cudf/core/column/string.py 86.79% <ø> (+0.30%) ⬆️
python/cudf/cudf/core/column/timedelta.py 88.57% <ø> (+0.33%) ⬆️
python/cudf/cudf/core/column_accessor.py 95.45% <ø> (+0.14%) ⬆️
python/cudf/cudf/core/dataframe.py 90.90% <ø> (+0.44%) ⬆️
... and 64 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 bb05ddc...d5c8212. 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.

Not a part of this PR but can you please add the stream to the group_offsets() and group_labels()

@mythrocks
Copy link
Contributor Author

Not a part of this PR but can you please add the stream to the group_offsets() and group_labels()

Done. Thanks for pointing that out. TIL.

@davidwendt
Copy link
Contributor

@mythrocks Is this good to merge?

@mythrocks
Copy link
Contributor Author

mythrocks commented Mar 20, 2021

Yikes. Yes, this should be merged. Or should have been. I'd better leave this till I've rebased and retested, in light of recent changes in the code base.

@mythrocks
Copy link
Contributor Author

Rebased and retested. All tests are passing. I'll merge this now.

@mythrocks
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 11455ee into rapidsai:branch-0.19 Mar 21, 2021
@mythrocks mythrocks deleted the fix-rolling-window-assert branch March 21, 2021 17:17
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 4 - Needs Review Waiting for reviewer to review or respond bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants