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

Consolidate SUM reductions #2381

Merged
merged 13 commits into from
Jul 24, 2024
Merged

Consolidate SUM reductions #2381

merged 13 commits into from
Jul 24, 2024

Conversation

mfoerste4
Copy link
Collaborator

This PR consists of multiple parts:

  1. redirect custom reduction kernels within stats namespace to linalg::reduce
  2. Specialize reduction kernels for addition utilizing the Kahan-Babushka-Neumaier-Sum link
  3. Slightly adjust kernel heuristics for coalesced reductions

This should address #2366 and #2205. With the kernel heuristics adjusted the maximum performance drop is 4%.

FYI, @tfeher

@mfoerste4 mfoerste4 requested a review from a team as a code owner July 16, 2024 13:33
@github-actions github-actions bot added the cpp label Jul 16, 2024
@mfoerste4 mfoerste4 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 16, 2024
tfeher
tfeher requested changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks Malte, this looks really good! I love how a new compensated reduction is reused among various stat functions! I just have a small request to improve documentation.

cpp/include/raft/linalg/detail/strided_reduction.cuh Show resolved Hide resolved
cpp/include/raft/linalg/detail/coalesced_reduction-inl.cuh Show resolved Hide resolved
@mfoerste4
Copy link
Collaborator Author

Thanks Malte, this looks really good! I love how a new compensated reduction is reused among various stat functions! I just have a small request to improve documentation.

Thanks @tfeher for reviewing. I added a note to both the implementations in detail as well as the main entry in raft::linalg::reduce.

tfeher
tfeher approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @mfoerste4 for the update, LGTM!

@mfoerste4
Copy link
Collaborator Author

@tfeher , sorry for the late change, I missed this before. The sample-parameter for mean/stdev was not implemented for column based data previously. I enabled it and updated the failing tests (that were sampling with rows == 1)

tfeher
tfeher approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Approving latest changes.

@mfoerste4
Copy link
Collaborator Author

This raised a bug in the test code that existed for a long time. IIUC the covariance algorithm using the pre-computed mean should always use the standard mean, no matter whether we are computing the sampled covariance or not.
A quick scan of the cuml brought up this code which might be wrong as well.

tfeher
tfeher approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks for catching these issues, LGTM.

@mfoerste4 mfoerste4 mentioned this pull request Jul 24, 2024
Open
@cjnolet
Copy link
Member

cjnolet commented Jul 24, 2024

/merge

@rapids-bot rapids-bot bot merged commit 759095a into rapidsai:branch-24.08 Jul 24, 2024
73 checks passed
@mfoerste4 mfoerste4 deleted the kahan_neumeier_sum branch July 24, 2024 19:16
@mfoerste4 mfoerste4 mentioned this pull request Jul 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfeher tfeher tfeher approved these changes

@cjnolet cjnolet cjnolet approved these changes

Assignees

@mfoerste4 mfoerste4

Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
VS/ML/DM Primitives Release Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mfoerste4 @cjnolet @tfeher