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

[REVIEW] Remove fixed_point precise round #6544

Merged
merged 7 commits into from
Oct 20, 2020

Conversation

codereport
Copy link
Contributor

@codereport codereport commented Oct 15, 2020

In working on the the fixed_point binary operations (#6528), we realized the "precise rounding" that we implemented for fixed_point adds increased complexity to implementing the binary_operation::DIV that would require non-trivial modifications to kernel.cpp and operations.h. We thererfore decided to revert these changes (this is consistent with the design of CNL) to avoid this complexity.

@codereport codereport added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. 4 - Needs Review Waiting for reviewer to review or respond labels Oct 15, 2020
@codereport codereport requested review from harrism and shwina October 15, 2020 23:36
@codereport codereport requested a review from a team as a code owner October 15, 2020 23:36
@codereport codereport self-assigned this Oct 15, 2020
@codereport codereport requested a review from nvdbaranec October 15, 2020 23:36
@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 Oct 16, 2020

Codecov Report

Merging #6544 into branch-0.17 will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.17    #6544   +/-   ##
============================================
  Coverage        83.12%   83.12%           
============================================
  Files               90       90           
  Lines            14824    14826    +2     
============================================
+ Hits             12322    12324    +2     
  Misses            2502     2502           
Impacted Files Coverage Δ
python/cudf/cudf/core/column/string.py 86.38% <0.00%> (+0.03%) ⬆️

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 78c1710...4f647d1. Read the comment docs.

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.

One big question, and two spelling corrections. otherwise looks fine.

cpp/include/cudf/fixed_point/fixed_point.hpp Show resolved Hide resolved
cpp/tests/fixed_point/fixed_point_tests.cu Outdated Show resolved Hide resolved
cpp/tests/fixed_point/fixed_point_tests.cu Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants