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 quantile gtests coded in namespace cudf::test #12049

Merged
merged 10 commits into from
Nov 7, 2022

Conversation

davidwendt
Copy link
Contributor

Description

Fixes cpp/tests/quantiles gtests source files coded in namespace cudf::test
The tdigest_utilities.cu was moved to cpp/tests/utilities since it is used by quantiles, groupby, reductions tests. Also, the header for the functions defined in this source file is in cpp/include/cudf_tests/.

The cpp/include/cudf_tests/tdigest_utilities.cuh was also including a source file header from cudf/tests/groupby which seemed odd and was corrected by moving the code it needed directly into the tdigest_utilities.cuh header. These functions were used by quantiles, groupby, reductions, etc so it made sense for them to be moved into this utility header.

Simple reworking some of the code in percentile_approx_test.cu allowed it to become a .cpp file as well.
Also made some minor changes to the tdigest_column_view class to isolate a functor inside the class instead of the namespace scope.

No function or test has changed just the source code reworked or moved around.

Reference #11734

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 2, 2022
@davidwendt davidwendt self-assigned this Nov 2, 2022
@github-actions github-actions bot added the CMake CMake build issue label Nov 2, 2022
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Base: 88.02% // Head: 88.04% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (403d716) compared to base (11b875b).
Patch has no changes to coverable lines.

❗ Current head 403d716 differs from pull request most recent head 19ecb3c. Consider uploading reports for the commit 19ecb3c to get more accurate results

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-22.12   #12049      +/-   ##
================================================
+ Coverage         88.02%   88.04%   +0.02%     
================================================
  Files               135      135              
  Lines             22025    22025              
================================================
+ Hits              19388    19393       +5     
+ Misses             2637     2632       -5     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/numerical.py 95.18% <0.00%> (-0.29%) ⬇️
python/cudf/cudf/core/dataframe.py 93.67% <0.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/string.py 88.65% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.51% <0.00%> (+0.20%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 93.75% <0.00%> (+0.96%) ⬆️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Nov 7, 2022
@davidwendt davidwendt marked this pull request as ready for review November 7, 2022 13:59
@davidwendt davidwendt requested review from a team as code owners November 7, 2022 13:59
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Approving C++ and CMake with two minor requests.

cpp/CMakeLists.txt Show resolved Hide resolved
cpp/tests/quantiles/quantile_test.cpp Outdated Show resolved Hide resolved
@davidwendt
Copy link
Contributor Author

davidwendt commented Nov 7, 2022

@gpucibot merge

@davidwendt davidwendt removed the request for review from harrism November 7, 2022 23:02
@rapids-bot rapids-bot bot merged commit f9a2512 into rapidsai:branch-22.12 Nov 7, 2022
@davidwendt davidwendt deleted the fix-gtests-ns-quantiles branch November 8, 2022 00:00
rapids-bot bot pushed a commit that referenced this pull request Nov 9, 2022
Changes `cudf::detail::tdigest` to `cudf::tdigest::detail` in the tdigest source files.
While working on #12049, found there was a mixture of `cudf::tdigest` and `cudf::detail::tdigest` that seemed confusing and inconsistent. Changing to `cudf::tdigest::detail` made this code easier to follow.
Also, move the `size_begin()` member function in `tdigest_column_view` out as a standalone function in a separate `.cuh` header since it is only used in a few places and the `tdigest_column_view.cuh` is included in many places. This allowed changing the `tdigest_column_view.cuh` to a `.hpp` file.

Depends on #12049

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Nghia Truong (https://github.com/ttnghia)
  - Ray Douglass (https://github.com/raydouglass)

URL: #12050
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 CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants