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

Use cuda::std::is_arithmetic in cudf::is_numeric trait. #9996

Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jan 7, 2022

The current implementation of cudf::is_numeric is equivalent to the implementation of cuda::std::is_arithmetic. This PR simplifies the implementation in cuDF and aligns it with libcudacxx.

Notes:

  • bool returns true from both (cuda::)std::is_integral and (cuda::)std::is_arithmetic, so bool is considered a "numeric" type. This behavior is unchanged.
  • We must use cuda::std::is_arithmetic rather than std::is_arithmetic to support 128-bit integer types (as we do with cuda::std::is_integral in the current implementation)

See also: https://en.cppreference.com/w/cpp/types/is_arithmetic

@bdice bdice added code quality libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 7, 2022
@bdice bdice self-assigned this Jan 7, 2022
@bdice bdice requested a review from a team as a code owner January 7, 2022 17:40
@bdice bdice requested review from cwharris and ttnghia January 7, 2022 17:40
@davidwendt
Copy link
Contributor

I think we should just use cuda::std::is_arithmetic directly and get rid of cudf::is_numeric. No need to have a cudf wrapper around a single std trait.

@bdice
Copy link
Contributor Author

bdice commented Jan 7, 2022

I think we should just use cuda::std::is_arithmetic directly and get rid of cudf::is_numeric. No need to have a cudf wrapper around a single std trait.

That's possible. @jrhemstad mentioned in conversation with me that it is nice that the "numeric" name aligns with make_numeric_column, among other "numeric" names (like numeric scalars). I'm open to whatever reviewers think is most appropriate here. Personally I lean towards keeping our own wrappers for ensuring internal consistency, since we define and use a lot of type traits that are not easily expressible in terms of those in <cuda/std/type_traits>.

@davidwendt
Copy link
Contributor

davidwendt commented Jan 7, 2022

That's fine. At the very least we could remove the cudf::is_numeric() that is only needed for the dispatch impl:


template <typename T>
constexpr inline bool is_numeric()
{
  return cuda::std::is_arithmetic<T>();
}

struct is_numeric_impl {
  template <typename T>
  constexpr bool operator()()
  {
    return cuda::std::is_arithmetic<T>();
  }
};

@bdice
Copy link
Contributor Author

bdice commented Jan 7, 2022

That's fine. At the very least we could remove the cudf::is_numeric() that is only needed for the dispatch impl:

I'm not sure if that's possible here -- lots of code appears to use the template form of this trait as well as the form accepting a data_type that does a type dispatch through is_numeric_impl: https://github.com/rapidsai/cudf/search?q=is_numeric (This search lists both cases, since GitHub code search ignores < and >.) Examples:

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #9996 (17be02a) into branch-22.02 (de8c0b8) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.02    #9996      +/-   ##
================================================
+ Coverage         10.41%   10.45%   +0.03%     
================================================
  Files               119      119              
  Lines             20023    20417     +394     
================================================
+ Hits               2086     2134      +48     
- Misses            17937    18283     +346     
Impacted Files Coverage Δ
python/cudf/cudf/io/hdf.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/feather.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/parquet.py 0.00% <0.00%> (ø)
... and 48 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 de8c0b8...17be02a. Read the comment docs.

@bdice
Copy link
Contributor Author

bdice commented Jan 11, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 07fa888 into rapidsai:branch-22.02 Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants