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

Hiding implementation details for random, stats, and matrix #356

Merged
merged 8 commits into from
Nov 11, 2021

Conversation

divyegala
Copy link
Member

No description provided.

@divyegala divyegala requested review from a team as code owners October 6, 2021 17:16
@github-actions github-actions bot added the cpp label Oct 6, 2021
@divyegala divyegala added 2 - In Progress Currenty a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 6, 2021
@divyegala divyegala added 3 - Ready for Review and removed 2 - In Progress Currenty a work in progress labels Oct 6, 2021
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 like a great start. We want to make sure any classes which are being used internally and contain kernel-specific things are also being hidden behind detail and exposed through a proxy in the public API.

cpp/include/raft/matrix/math.cuh Outdated Show resolved Hide resolved
cpp/include/raft/matrix/matrix.cuh Outdated Show resolved Hide resolved
cpp/include/raft/random/rng.cuh Show resolved Hide resolved
cpp/include/raft/stats/mean.cuh Outdated Show resolved Hide resolved
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.

Changes look great!

d_in[idx] = -d_in[idx];
}
}
detail::argmax(in, n_rows, n_cols, out, stream);
Copy link
Member

Choose a reason for hiding this comment

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

much better!

@cjnolet
Copy link
Member

cjnolet commented Oct 22, 2021

@divyegala looks like this got a tad out of date w/ the merge of the distances public API

@@ -0,0 +1,376 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

cugraph uses rng.cuh in a couple of places, so leaving a quick review so we don’t merge this before that side is ready (if it already is, just let me know and glad to approve/merge)

@dantegd
Copy link
Member

dantegd commented Nov 11, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 466ea71 into rapidsai:branch-21.12 Nov 11, 2021
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Nov 12, 2021
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Nov 12, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants