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

Enable rounding for Decimal32 and Decimal64 in cuDF #17332

Merged
merged 19 commits into from
Dec 10, 2024

Conversation

a-hirota
Copy link
Contributor

@a-hirota a-hirota commented Nov 15, 2024

Description

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Example of Behavior After Modification

The following example demonstrates the behavior of the round function for both float and Decimal32Dtype before and after the modification.

import cudf

def check(t):
    a = cudf.DataFrame({'a': [1.00, 2.054, 3.01]}).astype(t)
    print(type(t), a)
    a = a.round(decimals=1, how="half_up")
    print(type(t), a)

print('float')
check(float)
print('decimal')
check(cudf.Decimal32Dtype(precision=3, scale=2))

Output

After Modification

For Decimal32Dtype, rounding works as expected:

float
<class 'type'>        a
0  1.000
1  2.054
2  3.010
<class 'type'>      a
0  1.0
1  2.1
2  3.0

decimal
<class 'cudf.core.dtypes.Decimal32Dtype'>       a
0  1.00
1  2.05
2  3.01
<class 'cudf.core.dtypes.Decimal32Dtype'>      a
0  1.0
1  2.1
2  3.0

Before Modification

For Decimal32Dtype, rounding did not modify the values:

float
<class 'type'>        a
0  1.000
1  2.054
2  3.010
<class 'type'>      a
0  1.0
1  2.1
2  3.0

decimal
<class 'cudf.core.dtypes.Decimal32Dtype'>       a
0  1.00
1  2.05
2  3.01
<class 'cudf.core.dtypes.Decimal32Dtype'>       a
0  1.00
1  2.05
2  3.01

This example shows that, prior to this modification, rounding had no effect on Decimal32Dtype columns, while after the change, Decimal32Dtype columns round as expected.

Additional Information

  • Problem:
    cuDF currently does not support rounding for Decimal types, limiting its functionality compared to libcudf, which does support it.
  • Solution:
    Updated the cols definition in cudf/python/cudf/cudf/core/indexed_frame.py to allow rounding for Decimal32 and Decimal64 types. The modified code now checks if col.dtype is of Decimal32 or Decimal64 type, allowing rounding directly on these columns without making a copy.
  • Alternatives Considered:
    Using apply in Pandas can achieve similar functionality, but it is not as efficient as a native rounding method in cuDF.
  • Additional Context:

This enhancement improves consistency between cudf and libcudf by enabling direct rounding of Decimal types in cudf.

@a-hirota a-hirota requested a review from a team as a code owner November 15, 2024 00:28
Copy link

copy-pr-bot bot commented Nov 15, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the Python Affects Python cuDF API. label Nov 15, 2024
@vyasr
Copy link
Contributor

vyasr commented Nov 15, 2024

Thanks @a-hirota! Could you please add a test that would have failed with the old code that succeeds now? You should be able to construct the expected decimal column and then compare it to what you get out of rounding.

- Added test cases for "half_up" and "half_even" rounding modes.
- Tested both Decimal32Dtype and Decimal64Dtype with various precisions and scales.
- Ensured coverage for edge cases like .5 rounding to the nearest even number in "half_even".
- Verified correctness of rounding logic across different decimal places.
@a-hirota
Copy link
Contributor Author

Thank you @vyasr for your feedback!

I’ve added test cases that validate the rounding behavior for both Decimal32Dtype and Decimal64Dtype with the half_up and half_even rounding modes. These include scenarios where the previous code would have produced incorrect results, such as rounding .5 values inconsistently in the half_even mode. The expected output is constructed explicitly for each test case, and assertions ensure the correctness of the implementation.

Let me know if there's anything else you'd like me to adjust or add!

a-hirota and others added 3 commits November 16, 2024 12:58
Removed comments and print statements from the test function as requested.
Clarified the approach for handling "half_up" and "half_even" rounding methods.
@vyasr vyasr added feature request New feature or request non-breaking Non-breaking change labels Nov 19, 2024
@vyasr
Copy link
Contributor

vyasr commented Nov 19, 2024

/ok to test

@vyasr
Copy link
Contributor

vyasr commented Nov 20, 2024

@a-hirota looks like you need to fix the style, could you please run pre-commit?

a-hirota and others added 2 commits November 20, 2024 10:59
Ran pre-commit to address style issues as per review feedback. This ensures consistency with project coding standards.
@a-hirota
Copy link
Contributor Author

@vyasr
Apologies, I have run pre-commit.

@vyasr
Copy link
Contributor

vyasr commented Nov 20, 2024

No worries! Thank you.

@vyasr
Copy link
Contributor

vyasr commented Nov 20, 2024

/ok to test

@vyasr
Copy link
Contributor

vyasr commented Nov 20, 2024

/ok to test

@vyasr
Copy link
Contributor

vyasr commented Nov 20, 2024

@a-hirota it looks like pre-commit is still making changes to the test that you added.

@a-hirota
Copy link
Contributor Author

a-hirota commented Nov 21, 2024

@a-hirota it looks like pre-commit is still making changes to the test that you added.
@vyasr
Sorry, I forgot to push the test file...

@vyasr vyasr changed the base branch from branch-24.12 to branch-25.02 November 22, 2024 07:46
@vyasr
Copy link
Contributor

vyasr commented Nov 22, 2024

/ok to test

@vyasr
Copy link
Contributor

vyasr commented Dec 7, 2024

/ok to test

@vyasr vyasr requested a review from mroeschke December 7, 2024 00:50
Copy link
Contributor

@mroeschke mroeschke 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. Thanks @a-hirota

@mroeschke
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit 4764395 into rapidsai:branch-25.02 Dec 10, 2024
104 of 105 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants