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

Enable type conversion from float to decimal type #7450

Merged
merged 16 commits into from
Mar 10, 2021
Merged

Enable type conversion from float to decimal type #7450

merged 16 commits into from
Mar 10, 2021

Conversation

ChrisJar
Copy link
Contributor

@ChrisJar ChrisJar commented Feb 25, 2021 •
edited
Loading

This implements typecasting between decimal and float types.

Addresses half of #7440

@github-actions github-actions bot added the Python Affects Python cuDF API. label Feb 25, 2021
@ChrisJar ChrisJar changed the title [WIP] Enable type conversion from numerical to decimal type Enable type conversion from float to decimal type Mar 2, 2021
@ChrisJar ChrisJar marked this pull request as ready for review March 2, 2021 21:40
@ChrisJar ChrisJar requested a review from a team as a code owner March 2, 2021 21:40
@codecov
Copy link

codecov bot commented Mar 3, 2021 •
edited
Loading

Codecov Report

Merging #7450 (a0d9129) into branch-0.19 (53929eb) will increase coverage by 0.44%.
The diff coverage is 85.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7450      +/-   ##
===============================================
+ Coverage        81.88%   82.32%   +0.44%     
===============================================
  Files              101      101              
  Lines            16900    17283     +383     
===============================================
+ Hits             13838    14229     +391     
+ Misses            3062     3054       -8
Impacted Files Coverage Δ
python/cudf/cudf/core/column/decimal.py 93.33% <84.61%> (-2.50%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.85% <85.71%> (-0.17%) ⬇️
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/struct.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_csv.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_orc.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_json.py 100.00% <0.00%> (ø)
... and 39 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 53929eb...be6f019. Read the comment docs.

rgsl888prabhu
rgsl888prabhu reviewed Mar 3, 2021
View reviewed changes
python/cudf/cudf/_lib/unary.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/core/column/numerical.py
)
result = libcudf.unary.cast(self, dtype)
if isinstance(dtype, cudf.core.dtypes.Decimal64Dtype):
result.dtype.precision = dtype.precision
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it required to update precision in result ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The libcudf unary cast always returns a precision of 18 so it needs to be patched with the correct precision on the python side

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we are pretty much stuck with always patching the precision attribute after getting a decimal column back from libcudf :(

python/cudf/cudf/tests/test_decimal.py Outdated Show resolved Hide resolved
python/cudf/cudf/tests/test_decimal.py Show resolved Hide resolved
@rgsl888prabhu
Copy link
Contributor

This implements typecasting between decimal and float types.

Addresses half of #7440

This implements typecasting between decimal and float types.

Addresses half of #7440

When you said Addresses half of #7440, did you mean float <-> decimal type conversion and int <-> decimal type is yet to be addressed ?

@ChrisJar ChrisJar requested a review from rgsl888prabhu March 4, 2021 15:21
shwina
shwina approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@shwina shwina 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 to me! Approving with a request to add a TODO.

@shwina shwina added feature request New feature or request non-breaking Non-breaking change Cython labels Mar 8, 2021
@shwina
Copy link
Contributor

shwina commented Mar 8, 2021

@gpucibot merge

@ChrisJar
Copy link
Contributor Author

ChrisJar commented Mar 9, 2021

rerun tests

@shwina
Copy link
Contributor

shwina commented Mar 10, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8c44d62 into rapidsai:branch-0.19 Mar 10, 2021
hyperbolic2346 pushed a commit to hyperbolic2346/cudf that referenced this pull request Mar 25, 2021
@ChrisJar @hyperbolic2346
Enable type conversion from float to decimal type (rapidsai#7450)
af91aca 
This implements typecasting between `decimal` and `float` types.

Addresses half of rapidsai#7440

Authors:
  - @ChrisJar

Approvers:
  - Ram (Ramakrishna Prabhu) (@rgsl888prabhu)
  - Ashwin Srinath (@shwina)

URL: rapidsai#7450
@codereport codereport mentioned this pull request Sep 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgsl888prabhu rgsl888prabhu rgsl888prabhu approved these changes

@shwina shwina shwina approved these changes

@brandon-b-miller brandon-b-miller Awaiting requested review from brandon-b-miller

Assignees
No one assigned
Labels
feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChrisJar @rgsl888prabhu @shwina