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

Refactor thrust_copy_if into cudf::detail::copy_if_safe #12455

Merged
merged 8 commits into from
Jan 4, 2023

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Dec 30, 2022

This extracts thrust_copy_if out of json_tree.cu and puts it into cudf/detail/utilities/algorithm.cuh with a new name cudf::detail::copy_if_safe. Such utility is useful not just for use in json_tree.cu but potentially in many other places. The next immediate use case will be to implement from_json in NVIDIA/spark-rapids-jni#844.

This also changes the work in #12079 to adopt the new function cudf::detail::copy_if_safe.

@ttnghia ttnghia added feature request New feature or request 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS non-breaking Non-breaking change labels Dec 30, 2022
@ttnghia ttnghia requested a review from a team as a code owner December 30, 2022 17:09
@ttnghia ttnghia self-assigned this Dec 30, 2022
@ttnghia ttnghia added this to PR-WIP in v23.02 Release via automation Dec 30, 2022
@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Base: 86.58% // Head: 85.71% // Decreases project coverage by -0.86% ⚠️

Coverage data is based on head (cb836da) compared to base (b6dccb3).
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-23.02   #12455      +/-   ##
================================================
- Coverage         86.58%   85.71%   -0.87%     
================================================
  Files               155      155              
  Lines             24368    24798     +430     
================================================
+ Hits              21098    21255     +157     
- Misses             3270     3543     +273     
Impacted Files Coverage Δ
python/cudf/cudf/_version.py 1.41% <0.00%> (-98.59%) ⬇️
python/cudf/cudf/core/buffer/spill_manager.py 72.50% <0.00%> (-7.50%) ⬇️
python/cudf/cudf/core/buffer/spillable_buffer.py 90.04% <0.00%> (-2.81%) ⬇️
python/cudf/cudf/utils/dtypes.py 77.77% <0.00%> (-1.69%) ⬇️
python/cudf/cudf/options.py 86.11% <0.00%> (-1.59%) ⬇️
python/cudf/cudf/core/single_column_frame.py 94.30% <0.00%> (-1.27%) ⬇️
...ython/custreamz/custreamz/tests/test_dataframes.py 98.38% <0.00%> (-1.01%) ⬇️
python/dask_cudf/dask_cudf/io/parquet.py 91.81% <0.00%> (-0.59%) ⬇️
python/cudf/cudf/core/multiindex.py 91.66% <0.00%> (-0.51%) ⬇️
python/cudf/cudf/core/algorithms.py 90.00% <0.00%> (-0.48%) ⬇️
... and 36 more

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.

Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks!

v23.02 Release automation moved this from PR-WIP to PR-Reviewer approved Jan 3, 2023
@davidwendt
Copy link
Contributor

There is already a function named cudf::detail::copy_if() defined in include/cudf/detail/copy_if.cuh

std::unique_ptr<table> copy_if(

Perhaps this function needs a different name?

@ttnghia
Copy link
Contributor Author

ttnghia commented Jan 3, 2023

I thought about that too but didn't have a very good candidate name. How about safe_copy_if?

@davidwendt
Copy link
Contributor

I thought about that too but didn't have a very good candidate name. How about safe_copy_if?

We usually use safe as a suffix like the functions in here: https://github.com/rapidsai/cudf/blob/branch-23.02/cpp/include/cudf/detail/utilities/integer_utils.hpp
Perhaps copy_if_safe? That sounds more like "copy, if safe" instead of "copy-if, safe". Naming is hard.
I'm ok with either safe_copy_if or copy_if_safe.
Other options: blocked_copy_if or chunked_copy_if

@ttnghia
Copy link
Contributor Author

ttnghia commented Jan 3, 2023

We usually use safe as a suffix like the functions in here: https://github.com/rapidsai/cudf/blob/branch-23.02/cpp/include/cudf/detail/utilities/integer_utils.hpp Perhaps copy_if_safe? That sounds more like "copy, if safe" instead of "copy-if, safe". Naming is hard. I'm ok with either safe_copy_if or copy_if_safe. Other options: blocked_copy_if or chunked_copy_if

Then let's use copy_if_safe for consistency with other APIs having the _safe suffix.

@ttnghia ttnghia changed the title Refactor thrust_copy_if into cudf::detail::copy_if Refactor thrust_copy_if into cudf::detail::copy_if_safe Jan 3, 2023
@ttnghia
Copy link
Contributor Author

ttnghia commented Jan 4, 2023

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c1d8fde into rapidsai:branch-23.02 Jan 4, 2023
v23.02 Release automation moved this from PR-Reviewer approved to Done Jan 4, 2023
@ttnghia ttnghia deleted the thrust_copy_if branch January 4, 2023 14:59
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 feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants