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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 25 commits into from
Feb 19, 2021
Merged

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

merged 25 commits into from
Feb 19, 2021

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Feb 10, 2021 •
edited
Loading

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
Copy link

codecov bot commented Feb 10, 2021 •
edited
Loading

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
nvdbaranec
nvdbaranec requested changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@nvdbaranec nvdbaranec left a comment

Choose a reason for hiding this comment

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

Some small stuff.

cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated Show resolved Hide resolved
cpp/src/strings/convert/convert_fixed_point.cu Outdated Show resolved Hide resolved
cpp/src/strings/convert/convert_fixed_point.cu Show resolved Hide resolved
cpp/src/strings/convert/convert_fixed_point.cu Outdated Show resolved Hide resolved
cpp/src/strings/convert/convert_fixed_point.cu Outdated Show resolved Hide resolved
cpp/src/strings/convert/convert_fixed_point.cu Show resolved Hide resolved
kkraus14
kkraus14 approved these changes Feb 16, 2021
View reviewed changes
Copy link
Collaborator

@kkraus14 kkraus14 left a comment

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 requested changes Feb 17, 2021
View reviewed changes
Copy link
Member

@harrism harrism left a comment

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.

cpp/include/cudf/fixed_point/fixed_point.hpp Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_fixed_point.hpp Show resolved Hide resolved
@davidwendt davidwendt requested a review from harrism February 17, 2021 13:28
harrism
harrism approved these changes Feb 17, 2021
View reviewed changes
Copy link
Member

@harrism harrism left a comment

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.

cpp/include/cudf/strings/convert/convert_fixed_point.hpp Outdated Show resolved Hide resolved
codereport
codereport suggested changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@codereport codereport left a comment

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

cpp/tests/strings/fixed_point_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/strings/fixed_point_tests.cpp Show resolved Hide resolved
codereport
codereport approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@codereport codereport left a comment

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
Contributor Author

rerun tests

@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4a4172c into rapidsai:branch-0.19 Feb 19, 2021
@davidwendt davidwendt deleted the strings-to-fixed-point branch February 19, 2021 14:42
@davidwendt davidwendt mentioned this pull request Mar 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@codereport codereport codereport approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@kkraus14 kkraus14 kkraus14 approved these changes

@harrism harrism harrism approved these changes

Assignees

@davidwendt davidwendt

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Create a from_decimal formula for cudf::strings
6 participants
@davidwendt @harrism @kkraus14 @ajschmidt8 @codereport @nvdbaranec