Skip to content

Add string conversion to/from decimal values libcudf APIs - #7364

Merged
rapids-bot[bot] merged 25 commits into
NVIDIA:branch-0.19from
davidwendt:strings-to-fixed-point
Feb 19, 2021
Merged

Add string conversion to/from decimal values libcudf APIs#7364
rapids-bot[bot] merged 25 commits into
NVIDIA:branch-0.19from
davidwendt:strings-to-fixed-point

Conversation

@davidwendt

@davidwendt davidwendt commented Feb 10, 2021

Copy link
Copy Markdown
Contributor

Reference #7285
Closes #5818

This PR adds the libcudf APIs for converting to/from decimal32 and decimal64 column types.
PR also includes cudf::strings::is_fixed_point check API
While coding this, I found a bug in the fixed_point::string() operator and fixed it in this PR as well.

A follow on PR will include the Python/Cython interface to the new APIs.

@davidwendt davidwendt added feature request New feature or request 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Feb 10, 2021
@davidwendt davidwendt self-assigned this Feb 10, 2021
@github-actions github-actions Bot added the CMake CMake build issue label Feb 10, 2021
@github-actions github-actions Bot added the conda label Feb 10, 2021
@codecov

codecov Bot commented Feb 10, 2021

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (branch-0.19@593dc1c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.19    #7364   +/-   ##
==============================================
  Coverage               ?   82.23%           
==============================================
  Files                  ?      101           
  Lines                  ?    17060           
  Branches               ?        0           
==============================================
  Hits                   ?    14029           
  Misses                 ?     3031           
  Partials               ?        0           

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 593dc1c...c88f427. Read the comment docs.

@davidwendt
davidwendt marked this pull request as ready for review February 16, 2021 15:43
@davidwendt
davidwendt requested review from a team as code owners February 16, 2021 15:43
@davidwendt
davidwendt requested a review from harrism February 16, 2021 15:43
@davidwendt davidwendt removed the 2 - In Progress Currently a work in progress label Feb 16, 2021
@davidwendt davidwendt added the 3 - Ready for Review Ready for review by team label Feb 16, 2021

@nvdbaranec nvdbaranec left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some small stuff.

Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated
Comment thread cpp/src/strings/convert/convert_fixed_point.cu Outdated
Comment thread cpp/src/strings/convert/convert_fixed_point.cu
Comment thread cpp/src/strings/convert/convert_fixed_point.cu Outdated
Comment thread cpp/src/strings/convert/convert_fixed_point.cu Outdated
Comment thread cpp/src/strings/convert/convert_fixed_point.cu

@kkraus14 kkraus14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cmake / conda lgtm

@harrism harrism left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Just doc questions / suggestions.

Comment thread cpp/include/cudf/fixed_point/fixed_point.hpp
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp
Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp
@davidwendt
davidwendt requested a review from harrism February 17, 2021 13:28
Comment thread cpp/src/strings/convert/utilities.cuh Outdated
Comment thread cpp/src/strings/convert/convert_fixed_point.cu

@harrism harrism left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approve, but one small comment.

Comment thread cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated

@codereport codereport left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so far looks good, some unit test enhancements

Comment thread cpp/tests/strings/fixed_point_tests.cpp Outdated
Comment thread cpp/tests/strings/fixed_point_tests.cpp

@codereport codereport left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔥 looks great!

@davidwendt

Copy link
Copy Markdown
Contributor Author

rerun tests

@davidwendt

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 4a4172c into NVIDIA:branch-0.19 Feb 19, 2021
@davidwendt
davidwendt deleted the strings-to-fixed-point branch February 19, 2021 14:42
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 feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA] Create a from_decimal formula for cudf::strings

6 participants