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

Add decimal column handling in copy_type_metadata #7788

Merged
merged 12 commits into from
Apr 1, 2021

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Mar 31, 2021

  • All decimal columns returned by libcudf have a precision of 18 (Decimal64Dtype.MAX_PRECISION). The copy_type_metadata method is extended to copy the precision from the input column to the result as often we want the returned column to have the same precision as the input column.

  • In adding a test for this, I realized that the assert_eq utility is broken: it doesn't correctly check for dtype equality when the dtype is a cudf-specific type. Tacked on a fix.

@shwina shwina requested a review from a team as a code owner March 31, 2021 16:06
@github-actions github-actions bot added the Python Affects Python cuDF API. label Mar 31, 2021
Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

Thanks for this fix Ashwin 🙏

Can we now remove all explicit dtype checks too in the test_decimal file in this PR ? https://github.com/rapidsai/cudf/blob/branch-0.19/python/cudf/cudf/tests/test_decimal.py#L91

@shwina
Copy link
Contributor Author

shwina commented Mar 31, 2021

Will do!

@shwina shwina added non-breaking Non-breaking change bug Something isn't working labels Mar 31, 2021
@shwina
Copy link
Contributor Author

shwina commented Mar 31, 2021

rerun tests

Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

lgtm!

@galipremsagar galipremsagar added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Mar 31, 2021
@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #7788 (aab045d) into branch-0.19 (6cab04a) will increase coverage by 0.41%.
The diff coverage is 100.00%.

❗ Current head aab045d differs from pull request most recent head 389d14b. Consider uploading reports for the commit 389d14b to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7788      +/-   ##
===============================================
+ Coverage        82.26%   82.68%   +0.41%     
===============================================
  Files              103      103              
  Lines            17201    17572     +371     
===============================================
+ Hits             14151    14530     +379     
+ Misses            3050     3042       -8     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 100.00% <100.00%> (ø)
python/cudf/cudf/core/column/column.py 87.63% <100.00%> (+0.12%) ⬆️
python/cudf/cudf/core/column/decimal.py 92.85% <100.00%> (+0.17%) ⬆️
python/cudf/cudf/core/dtypes.py 93.80% <100.00%> (+0.68%) ⬆️
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%> (ø)
... and 42 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 6cab04a...389d14b. Read the comment docs.

@kkraus14
Copy link
Collaborator

kkraus14 commented Apr 1, 2021

rerun tests

@kkraus14
Copy link
Collaborator

kkraus14 commented Apr 1, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit ed469ca into rapidsai:branch-0.19 Apr 1, 2021
rapids-bot bot pushed a commit that referenced this pull request Apr 2, 2021
This enables joins on decimal columns with the same precision and scale.

Closes #7497 
Depends on #7788

Authors:
  - https://github.com/ChrisJar

Approvers:
  - Keith Kraus (https://github.com/kkraus14)
  - Ashwin Srinath (https://github.com/shwina)
  - https://github.com/brandon-b-miller
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #7764
shwina pushed a commit to shwina/cudf that referenced this pull request Apr 7, 2021
This enables joins on decimal columns with the same precision and scale.

Closes rapidsai#7497 
Depends on rapidsai#7788

Authors:
  - https://github.com/ChrisJar

Approvers:
  - Keith Kraus (https://github.com/kkraus14)
  - Ashwin Srinath (https://github.com/shwina)
  - https://github.com/brandon-b-miller
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: rapidsai#7764
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants