-
Notifications
You must be signed in to change notification settings - Fork 915
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
Change cudf::detail::tdigest to cudf::tdigest::detail #12050
Change cudf::detail::tdigest to cudf::tdigest::detail #12050
Conversation
Codecov ReportBase: 87.47% // Head: 88.07% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-22.12 #12050 +/- ##
================================================
+ Coverage 87.47% 88.07% +0.59%
================================================
Files 133 135 +2
Lines 21826 22025 +199
================================================
+ Hits 19093 19398 +305
+ Misses 2733 2627 -106
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
This is a potential improvement with documentation: We need to standardize either cudf::detail::module_name
or cudf::module_name::detail
namespace. From this PR, cudf::module_name::detail
should be favored and published into the dev guide.
This also came up in #12046 (comment). |
I'm planning to open an issue for cuIO (which is mostly in the wrong order of namespaces). Let me know if this is something we need to fix in the rest of libcudf as well, so the issue reflects that too. |
@gpucibot merge |
Description
Changes
cudf::detail::tdigest
tocudf::tdigest::detail
in the tdigest source files.While working on #12049, found there was a mixture of
cudf::tdigest
andcudf::detail::tdigest
that seemed confusing and inconsistent. Changing tocudf::tdigest::detail
made this code easier to follow.Also, move the
size_begin()
member function intdigest_column_view
out as a standalone function in a separate.cuh
header since it is only used in a few places and thetdigest_column_view.cuh
is included in many places. This allowed changing thetdigest_column_view.cuh
to a.hpp
file.Depends on #12049
Checklist