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

Improvement of the math API wrappers #1146

Merged
merged 17 commits into from
Jan 21, 2023

Conversation

Nyrio
Copy link
Contributor

@Nyrio Nyrio commented Jan 16, 2023

Solves #1025

Provides a centralized collection of host- and device-friendly wrappers around common math operations, with generalizations when useful. Deprecates former myXxx wrappers.

Those wrappers are mostly intended to future-proof the API as well as simplify the definition of host-device functions.

@Nyrio Nyrio requested review from a team as code owners January 16, 2023 18:31
@Nyrio Nyrio added 3 - Ready for Review improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 16, 2023
@Nyrio Nyrio self-assigned this Jan 18, 2023
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

So nice to see all these my* types being removed, the math functions consolidated, and now even tests for the use of the wrappers in both host and device functions.

My feedback is mostly for documentation consistency, otherwise I think the changes look great.

cpp/include/raft/core/math.hpp Outdated Show resolved Hide resolved
cpp/include/raft/core/math.hpp Show resolved Hide resolved
cpp/include/raft/core/math.hpp Show resolved Hide resolved
@Nyrio Nyrio requested a review from cjnolet January 20, 2023 13:21
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks again for consolidating these

@codecov-commenter
Copy link

Codecov Report

Base: 87.99% // Head: 87.99% // No change to project coverage 👍

Coverage data is based on head (128e50c) compared to base (d233a2c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-23.02    #1146   +/-   ##
=============================================
  Coverage         87.99%   87.99%           
=============================================
  Files                21       21           
  Lines               483      483           
=============================================
  Hits                425      425           
  Misses               58       58           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cjnolet
Copy link
Member

cjnolet commented Jan 21, 2023

/merge

@rapids-bot rapids-bot bot merged commit a9e1adc into rapidsai:branch-23.02 Jan 21, 2023
ahendriksen pushed a commit to ahendriksen/raft that referenced this pull request Jan 23, 2023
Solves rapidsai#1025 

Provides a centralized collection of host- and device-friendly wrappers around common math operations, with generalizations when useful. Deprecates former `myXxx` wrappers.

Those wrappers are mostly intended to future-proof the API as well as simplify the definition of host-device functions.

Authors:
  - Louis Sugy (https://github.com/Nyrio)
  - Corey J. Nolet (https://github.com/cjnolet)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: rapidsai#1146
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

3 participants