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] Enable fixed_point binary operations #6528

Merged
merged 16 commits into from
Oct 24, 2020

Conversation

codereport
Copy link
Contributor

@codereport codereport commented Oct 14, 2020

This addresses are part of #3556. It is a follow up to #5861 where most of the work was done. However, this PR fixes an issue with binary_operator::DIV.

Starting point is to comment in the commented out unit tests in binop-integration-tests.cpp and go from there.

Update:

After #6544 got merged, this job was simplified to the following:

  • Enable bool is_supported_fixed_point_binop(binary_operator op)
  • Uncomment out tests
  • Modify FixedPointBinaryOpDiv test
  • Add a couple more binary_operator::DIV tests

@codereport codereport added the 2 - In Progress Currently a work in progress label Oct 14, 2020
@codereport codereport requested a review from a team as a code owner October 14, 2020 15:53
@codereport codereport added the libcudf Affects libcudf (C++/CUDA) code. label Oct 14, 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 Oct 14, 2020

Codecov Report

Merging #6528 into branch-0.17 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.17    #6528   +/-   ##
============================================
  Coverage        82.72%   82.72%           
============================================
  Files               90       90           
  Lines            14892    14892           
============================================
  Hits             12320    12320           
  Misses            2572     2572           

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 17cd94f...cfc6d30. Read the comment docs.

@harrism
Copy link
Member

harrism commented Oct 14, 2020

Please target branch-0.17.

@harrism harrism changed the base branch from branch-0.16 to branch-0.17 October 14, 2020 21:09
@sperlingxx
Copy link
Contributor

For current implementation, it seems that fixed_point types are not able to perform binary opeartion with other numeric types. Will we enable it in this ongoing PR?

@codereport
Copy link
Contributor Author

For current implementation, it seems that fixed_point types are not able to perform binary opeartion with other numeric types. Will we enable it in this ongoing PR?

No, that is not a part of this PR.

@harrism
Copy link
Member

harrism commented Oct 15, 2020

@sperlingxx Once (unary) casts are implemented the approach would be to cast to fixed point and then do the binary operation.

@sperlingxx
Copy link
Contributor

@sperlingxx Once (unary) casts are implemented the approach would be to cast to fixed point and then do the binary operation.

I got it. Thanks, Mark!

Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

This is from a previous PR but during the creation of temporary result in fixed_point_binary_operation, cudf::binary_operation is being used to create a temporary column of the same scale. This does not take a stream. Try using the detail version.

cpp/src/binaryop/binaryop.cpp Show resolved Hide resolved
@codereport codereport added the 0 - Blocked Cannot progress due to external reasons label Oct 20, 2020
@codereport
Copy link
Contributor Author

This job is currently waiting on #6544 to be merged.

@codereport codereport removed the 0 - Blocked Cannot progress due to external reasons label Oct 20, 2020
@codereport codereport changed the title [WIP] Enable fixed_point binary operations [REVIEW] Enable fixed_point binary operations Oct 20, 2020
@codereport codereport requested a review from devavret October 20, 2020 21:47
@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 Oct 20, 2020
@codereport codereport added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Oct 24, 2020
@codereport codereport merged commit 01d106e into rapidsai:branch-0.17 Oct 24, 2020
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 libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants