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

Missing device_storage_dispatch change affecting cudf::gather #7449

Merged
merged 3 commits into from
Feb 25, 2021

Conversation

codereport
Copy link
Contributor

Resolves #7441

Missed a necessary change to type_dispatcher invocation in /lists/copyting/gather.cu in #7419

@codereport codereport added bug Something isn't working 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. 4 - Needs Review Waiting for reviewer to review or respond non-breaking Non-breaking change labels Feb 25, 2021
@codereport codereport self-assigned this Feb 25, 2021
@codereport codereport requested a review from a team as a code owner February 25, 2021 04:50
@codereport
Copy link
Contributor Author

rerun tests

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. Just update the copyright in cpp/src/lists/copying/gather.cu

@revans2
Copy link
Contributor

revans2 commented Feb 25, 2021

Thanks this fixed most of the issues I was seeing. I am still seeing a problem with window functions when doing a collect list on structs with decimal values in them.

@mythrocks does the collect list functionality use a scatter or a gather?

@codereport
Copy link
Contributor Author

Thanks this fixed most of the issues I was seeing. I am still seeing a problem with window functions when doing a collect list on structs with decimal values in them.

@mythrocks does the collect list functionality use a scatter or a gather?

Do you have a test I can used to test in libcudf? Or just telling which API and the exact conditions and I can write one.

@revans2
Copy link
Contributor

revans2 commented Feb 25, 2021

I have not verified yet if this test passes before the change, but it fails now.

diff --git a/cpp/tests/collect_list/collect_list_test.cpp b/cpp/tests/collect_list/collect_list_test.cpp
index 98a7b2bacc..f50061aeb9 100644
--- a/cpp/tests/collect_list/collect_list_test.cpp
+++ b/cpp/tests/collect_list/collect_list_test.cpp
@@ -42,7 +42,7 @@ struct TypedCollectListTest : public CollectListTest {
 };
 
 using TypesForTest = cudf::test::
-  Concat<cudf::test::IntegralTypes, cudf::test::FloatingPointTypes, cudf::test::DurationTypes>;
+  Concat<cudf::test::IntegralTypes, cudf::test::FloatingPointTypes, cudf::test::DurationTypes, cudf::test::FixedPointTypes>;
 
 TYPED_TEST_CASE(TypedCollectListTest, TypesForTest);
 

@revans2
Copy link
Contributor

revans2 commented Feb 25, 2021

Just verified that the changes to collect_list_test.cpp pass if go back

@codereport
Copy link
Contributor Author

Just verified that the changes to collect_list_test.cpp pass if go back

@revans2 Just pushed a second fix. Let me know if this resolves the test / if there are any remaining.

@revans2
Copy link
Contributor

revans2 commented Feb 25, 2021

I'm building it now and will let you know when I have results

@revans2
Copy link
Contributor

revans2 commented Feb 25, 2021

+1 this fixed the last test that we had failing. I will rerun all of our tests just to be sure, but it is looking a lot better.

Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

All of my tests passed

@codereport codereport added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Feb 25, 2021
@codereport
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c80f9db into rapidsai:branch-0.19 Feb 25, 2021
@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #7449 (b90197f) into branch-0.19 (43b44e1) will increase coverage by 0.43%.
The diff coverage is 96.66%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7449      +/-   ##
===============================================
+ Coverage        81.80%   82.24%   +0.43%     
===============================================
  Files              101      101              
  Lines            16695    17069     +374     
===============================================
+ Hits             13658    14039     +381     
+ Misses            3037     3030       -7     
Impacted Files Coverage Δ
python/cudf/cudf/core/frame.py 89.35% <ø> (+0.09%) ⬆️
python/cudf/cudf/core/column_accessor.py 95.47% <95.65%> (+2.53%) ⬆️
python/cudf/cudf/core/dataframe.py 90.58% <100.00%> (+0.12%) ⬆️
python/cudf/cudf/utils/gpu_utils.py 53.65% <0.00%> (-4.88%) ⬇️
python/cudf/cudf/core/abc.py 87.23% <0.00%> (-1.14%) ⬇️
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%> (ø)
python/cudf/cudf/core/column/struct.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
... and 43 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 3c8b831...b90197f. Read the comment docs.

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 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] gather list of decimal values now fails
4 participants