-
Notifications
You must be signed in to change notification settings - Fork 514
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
float64 support in multi-sum and child_index() #4648
Conversation
- removed default T=float from multi-sum - tests with float64 for multi-sum and child_index()
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #4648 +/- ##
===============================================
Coverage ? 83.86%
===============================================
Files ? 251
Lines ? 20293
Branches ? 0
===============================================
Hits ? 17018
Misses ? 3275
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
cpp/test/sg/fil_child_index_test.cu
Outdated
operator dense_node<float>() | ||
{ | ||
return dense_node<float>({}, split<float>(), fid, def_left, false, is_categorical); | ||
} | ||
operator dense_node<double>() | ||
{ | ||
return dense_node<double>({}, split<double>(), fid, def_left, false, is_categorical); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to write this as a single template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks for the suggestion!
cpp/test/sg/fil_child_index_test.cu
Outdated
return sparse_node16<float>({}, split<float>(), fid, def_left, false, is_categorical, left); | ||
} | ||
operator sparse_node8() | ||
operator sparse_node16<double>() | ||
{ | ||
return sparse_node8({}, split(), fid, def_left, false, is_categorical, left); | ||
return sparse_node16<double>({}, split<double>(), fid, def_left, false, is_categorical, left); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: Any reason not to use a single template for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a very small suggestion that is probably not performance-critical as I notice most of this PR is testing changes. LGTM
Co-authored-by: Divye Gala <divyegala@gmail.com>
@wphicks I've addressed all of your comments. Could you take another look? |
rerun tests |
rerun tests |
@gpucibot merge |
rerun tests |
2 similar comments
rerun tests |
rerun tests |
- removed default `T = float` from multi-sum - tests with float64 for multi-sum and `child_index()` - refactored multi-sum tests to reduce shared memory usage - this is needed for the tests with float64 to compile This is pull request 1 of 3 to integrate rapidsai#4646. This pull request is partly based on the work by @levsnv. Authors: - Andy Adinets (https://github.com/canonizer) Approvers: - Divye Gala (https://github.com/divyegala) - William Hicks (https://github.com/wphicks) URL: rapidsai#4648
- removed default `T = float` from multi-sum - tests with float64 for multi-sum and `child_index()` - refactored multi-sum tests to reduce shared memory usage - this is needed for the tests with float64 to compile This is pull request 1 of 3 to integrate rapidsai#4646. This pull request is partly based on the work by @levsnv. Authors: - Andy Adinets (https://github.com/canonizer) Approvers: - Divye Gala (https://github.com/divyegala) - William Hicks (https://github.com/wphicks) URL: rapidsai#4648
T = float
from multi-sumchild_index()
This is pull request 1 of 3 to integrate #4646. This pull request is partly based on the work by @levsnv.