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

[REVIEW] Implement cudf::round decimal32 & decimal64 (HALF_UP and HALF_EVEN) #6685

Merged
merged 17 commits into from
Nov 8, 2020
Merged

[REVIEW] Implement cudf::round decimal32 & decimal64 (HALF_UP and HALF_EVEN) #6685

merged 17 commits into from
Nov 8, 2020

Conversation

codereport
Copy link
Contributor

@codereport codereport commented Nov 5, 2020 •
edited
Loading

This PR is the final of 3 PRs and resolves #3790.

  1. Implement cudf::round floating point and integer types (HALF_UP) [REVIEW] Implement cudf::round floating point and integer types (HALF_UP) [skip ci] #6562
  2. Implement cudf::round floating point and integer types (HALF_EVEN) [REVIEW] Implement cudf::round floating point and integer types (HALF_EVEN) #6647
  3. Implement cudf::round decimal32 & decimal64 (HALF_UP and HALF_EVEN) ◀️

The PR to do list is as follows:

  • Add code for adjusted type of resulting column
  • Add implementation for half_up positive
  • Add implementation for half_up zero
  • Add implementation for half_up negative
  • Add implementation for half_even positive
  • Add implementation for half_even zero
  • Add implementation for half_even negative
  • Update documentation
  • Add basic unit tests
  • Add comprehensive unit tests
  • Refactor leveraging the factor that EVERY fixed_point function object is identical 🔥

@codereport codereport added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. labels Nov 5, 2020
@codereport codereport self-assigned this Nov 5, 2020
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@codecov
Copy link

codecov bot commented Nov 5, 2020 •
edited
Loading

Codecov Report

Merging #6685 (0b421a0) into branch-0.17 (2cf893c) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.17    #6685      +/-   ##
===============================================
- Coverage        82.33%   82.28%   -0.05%     
===============================================
  Files               94       94              
  Lines            15369    15436      +67     
===============================================
+ Hits             12654    12702      +48     
- Misses            2715     2734      +19
Impacted Files Coverage Δ
python/cudf/cudf/io/orc.py 81.13% <0.00%> (-16.49%) ⬇️
python/cudf/cudf/io/csv.py 95.12% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/utils/ioutils.py 86.36% <0.00%> (+0.25%) ⬆️

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 2cf893c...0b421a0. Read the comment docs.

@codereport codereport changed the title [WIP] Implement cudf::round decimal32 & decimal64 (HALF_UP and HALF_EVEN) [REVIEW] Implement cudf::round decimal32 & decimal64 (HALF_UP and HALF_EVEN) Nov 5, 2020
@codereport codereport added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond and removed 2 - In Progress Currently a work in progress labels Nov 5, 2020
@codereport codereport marked this pull request as ready for review November 5, 2020 20:53
@codereport codereport requested a review from a team as a code owner November 5, 2020 20:53
nvdbaranec
nvdbaranec reviewed Nov 5, 2020
View reviewed changes
cpp/src/round/round.cu Outdated Show resolved Hide resolved
nvdbaranec
nvdbaranec requested changes Nov 5, 2020
View reviewed changes
cpp/src/round/round.cu Outdated Show resolved Hide resolved
harrism
harrism reviewed Nov 6, 2020
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.

Just a couple typos, really.

cpp/include/cudf/round.hpp Outdated Show resolved Hide resolved
cpp/src/round/round.cu Outdated Show resolved Hide resolved
cpp/src/round/round.cu Show resolved Hide resolved
codereport and others added 2 commits November 5, 2020 20:46
@codereport @harrism
Update cpp/src/round/round.cu
05cf89c 
Co-authored-by: Mark Harris <mharris@nvidia.com>
@codereport
Docs clarification and cleanup
0b421a0
@codereport
Copy link
Contributor Author

rerun tests

@harrism harrism merged commit 3d44ed5 into rapidsai:branch-0.17 Nov 8, 2020
@codereport codereport mentioned this pull request Nov 9, 2020
Closed
45 tasks
@tgravescs tgravescs mentioned this pull request Nov 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrism harrism harrism approved these changes

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@trxcllnt trxcllnt Awaiting requested review from trxcllnt

Assignees

@codereport codereport

Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Support HALF_UP and HALF_EVEN rounding to a set number of decimal places
4 participants
@codereport @GPUtester @harrism @nvdbaranec