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

[FEA] Provide fixed-point support for MOD, PMOD and TRUE_DIV #7132

Closed
nartal1 opened this issue Jan 12, 2021 · 12 comments
Closed

[FEA] Provide fixed-point support for MOD, PMOD and TRUE_DIV #7132

nartal1 opened this issue Jan 12, 2021 · 12 comments
Assignees
@codereport
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS
Milestone
Decimal data type...

Comments

@nartal1
Copy link
Member

nartal1 commented Jan 12, 2021

Is your feature request related to a problem? Please describe.
Fixed-point support missing for MOD, PMOD and TRUE_DIV.

Describe the solution you'd like
We should support Fixed-point for MOD, PMOD and TRUE_DIV operations.

Additional context
This issue builds upon #3556
@nartal1 nartal1 added feature request New feature or request Needs Triage Need team to review and classify Spark Functionality that helps Spark RAPIDS labels Jan 12, 2021
This was referenced Jan 12, 2021
Closed
Closed
@harrism
Copy link
Member

harrism commented Jan 19, 2021

@codereport Assigning you, though this is likely to be 0.19.

@harrism harrism mentioned this issue Jan 20, 2021
Closed
45 tasks
@revans2 revans2 mentioned this issue Jan 22, 2021
Closed
3 tasks
@codereport
Copy link
Contributor

codereport commented Jan 22, 2021 •
edited
Loading

Just spoke with @revans2, here is a summary of our conversation:

  • Currently, libcudf fixed_point only supports DIV, which calculates resulting scale as s1 - s2
  • Java/Spark/Python have TRUE_DIV which calculates resulting scale as max(6, s1 + p2 + 1)
  • libcudf fixed_point has no concept of TRUE_DIV and also no concept of precision

Note: s1 = scale_lhs, s2 = scale_rhs, p2 = precision_rhs

Suggested Path Forward to Support TRUE_DIV:
Have the cudf::binary_operation for fixed_point TRUE_DIV require resulting scale to be passed in (instead of calculating it on its own). This will work for both Python, Spark/Java and doesn't conflict with anything in C++ - and would be relatively straight forward to implement.

@harrism, @jrhemstad If you don't object ... I will move forward with this plan for TRUE_DIV

@harrism
Copy link
Member

harrism commented Jan 22, 2021

Sounds like a good plan.

@codereport codereport mentioned this issue Jan 22, 2021
Merged
7 tasks
@kkraus14 kkraus14 added libcudf Affects libcudf (C++/CUDA) code. and removed Needs Triage Need team to review and classify labels Jan 27, 2021
@kkraus14
Copy link
Collaborator

cc @shwina for visibility related to what's needed in the Python side for the binop

rapids-bot bot pushed a commit that referenced this issue Jan 29, 2021
@codereport
Add support for cudf::binary_operation TRUE_DIV for decimal32 a…
b097b5a 
…nd `decimal64` (#7198)

This resolves a part of #7132

**ToDo:**
* [x] Simple unit test
* [x] Comprehensive unit tests
* [x] Initial Column + Column
* [x] Full Column + Column
* [x] Column + Scalar
* [x] Scalar + Column
* [x] Cleanup

Authors:
  - Conor Hoekstra (@codereport)

Approvers:
  - Mark Harris (@harrism)
  - @nvdbaranec

URL: #7198
@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@revans2
Copy link
Contributor

revans2 commented Mar 3, 2021

this is still desired

@github-actions
Copy link

github-actions bot commented Apr 2, 2021

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@nartal1
Copy link
Member Author

nartal1 commented Apr 2, 2021

this still needs to be addressed.

@github-actions
Copy link

github-actions bot commented May 5, 2021

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@beckernick
Copy link
Member

Are the outstanding elements in this feature request MOD and PMOD?

@nartal1
Copy link
Member Author

nartal1 commented Jun 17, 2021

Yes, that's correct.

@codereport codereport mentioned this issue Feb 1, 2022
Merged
5 tasks
rapids-bot bot pushed a commit that referenced this issue Feb 3, 2022
@codereport
Support for MOD, PMOD and PYMOD for decimal32/64/128 (#10179)
f4ac6d4 
Resolves `libcudf` `MOD` and `PMOD` portion of #7132

To Do:
* [x] `MOD`
* [x] `PMOD`
* [x] `PYMOD`
* [x] Unit tests
* [x] Different scaled tests

Authors:
  - Conor Hoekstra (https://github.com/codereport)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #10179
@GregoryKimball
Copy link
Contributor

Closed by #10179

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@codereport codereport

Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS
Projects
None yet
Milestone
Decimal data type and operations
Development

No branches or pull requests
7 participants
@harrism @revans2 @kkraus14 @beckernick @GregoryKimball @codereport @nartal1