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 scatter output size for structs. #8155

Merged
merged 1 commit into from
May 4, 2021

Conversation

mythrocks
Copy link
Contributor

@mythrocks mythrocks commented May 4, 2021

Fixes #8150, which causes CUDF failures if the scatter() source struct column has a different size from that of the target. It turns out that the implementation was erroneously using the source column's size to construct the result, instead of the target's.

The following changes were made in this commit:

  1. Scatter output's size was set to target's size instead of source.
  2. Test case for the above.
  3. Peripherally, scatter_lists_test and scatter_struct_test were moved from .cu to .cpp, for faster compile.

1. Output struct size must match target column, not source.
2. Test case for rapidsai#1.
3. Also, move scatter_struct_tests and scatter_list_tests
   from .cu to .cpp, for faster compile.
@mythrocks mythrocks requested a review from a team as a code owner May 4, 2021 19:05
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels May 4, 2021
@mythrocks mythrocks requested a review from ttnghia May 4, 2021 19:05
@mythrocks mythrocks self-assigned this May 4, 2021
@mythrocks mythrocks added 3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change labels May 4, 2021
@mythrocks mythrocks removed the request for review from harrism May 4, 2021 19:27
@mythrocks
Copy link
Contributor Author

I'd better let @harrism off the hook on this one. Thanks for the reviews, @ttnghia && @davidwendt.

@mythrocks mythrocks added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels May 4, 2021
@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #8155 (349293d) into branch-0.20 (51336df) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 349293d differs from pull request most recent head 24eaba0. Consider uploading reports for the commit 24eaba0 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.20    #8155      +/-   ##
===============================================
+ Coverage        82.88%   82.92%   +0.03%     
===============================================
  Files              103      104       +1     
  Lines            17668    17896     +228     
===============================================
+ Hits             14645    14840     +195     
- Misses            3023     3056      +33     
Impacted Files Coverage Δ
python/cudf/cudf/core/abc.py 91.66% <ø> (+0.17%) ⬆️
python/cudf/cudf/core/algorithms.py 82.35% <ø> (ø)
python/cudf/cudf/core/column/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py 92.37% <ø> (+0.13%) ⬆️
python/cudf/cudf/core/column/column.py 88.20% <ø> (-0.44%) ⬇️
python/cudf/cudf/core/column/datetime.py 88.03% <ø> (-1.88%) ⬇️
python/cudf/cudf/core/column/decimal.py 91.04% <ø> (-1.89%) ⬇️
python/cudf/cudf/core/column/interval.py 91.66% <ø> (+0.55%) ⬆️
python/cudf/cudf/core/column/lists.py 86.98% <ø> (-0.43%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.72% <ø> (+0.29%) ⬆️
... and 35 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 d56428a...24eaba0. Read the comment docs.

@mythrocks
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a80dff2 into rapidsai:branch-0.20 May 4, 2021
@mythrocks mythrocks deleted the fix-scatter-struct branch May 4, 2021 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Calling scatter() on STRUCT column causes CUDF_FAIL()
3 participants