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

Remove cutlass usage in row major input for euclidean exp/unexp, cosine and L1 distance matrix #3589

Merged
merged 10 commits into from
Mar 26, 2021
Merged

Remove cutlass usage in row major input for euclidean exp/unexp, cosine and L1 distance matrix #3589

merged 10 commits into from
Mar 26, 2021

Conversation

mdoijade
Copy link
Contributor

@mdoijade mdoijade commented Mar 8, 2021

Remove cutlass usage in row major input for euclidean exp/unexp distance matrix calculation.
make use of contraction kernels for gemm NT calculation.
add PairwiseDistances class which will serve as common class for other distance metrics like cosine as well

@mdoijade
Remove cutlass usage in row major input for euclidean exp/unexp dista…
c0ceb93 
…nce matrix calculation. make use of contraction kernels for gemm NT calculation. add PairwiseDistances class which will serve as common class for other distance metrics like cosine as well
@mdoijade mdoijade requested review from a team as code owners March 8, 2021 10:30
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@mdoijade
Remove cutlass usage in row major input for cosine distance matrix ca…
c3fbbac 
…lculation. instead use PairwiseDistances/contraction class kernel for it
@mdoijade mdoijade changed the title Remove cutlass usage in row major input for euclidean exp/unexp distance matrix Remove cutlass usage in row major input for euclidean exp/unexp and cosine distance matrix Mar 9, 2021
@mdoijade
Remove cutlass usage in row major input for L1 distance matrix calcul…
707fdb0 
…ation. instead use PairwiseDistances/contraction class kernel for it
@teju85 teju85 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 15, 2021
@JohnZed
Copy link
Contributor

JohnZed commented Mar 15, 2021

Ok to test

@JohnZed
Copy link
Contributor

JohnZed commented Mar 15, 2021

Add to allowlist

@mdoijade mdoijade changed the title Remove cutlass usage in row major input for euclidean exp/unexp and cosine distance matrix Remove cutlass usage in row major input for euclidean exp/unexp, cosine and L1 distance matrix Mar 15, 2021
teju85
teju85 requested changes Mar 17, 2021
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

Just started reviewing this code. Some initial comments while I go through the rest of the PR.

cpp/src_prims/distance/pairwise_distance_base.cuh Outdated Show resolved Hide resolved
cpp/src_prims/distance/pairwise_distance_base.cuh Outdated Show resolved Hide resolved
cpp/src_prims/distance/pairwise_distance_base.cuh Show resolved Hide resolved
cpp/src_prims/distance/pairwise_distance_base.cuh Show resolved Hide resolved
teju85
teju85 requested changes Mar 17, 2021
View reviewed changes
cpp/src_prims/distance/cosine.cuh Outdated Show resolved Hide resolved
cpp/src_prims/distance/cosine.cuh Outdated Show resolved Hide resolved
cpp/src_prims/distance/cosine.cuh Outdated Show resolved Hide resolved
cpp/src_prims/distance/euclidean.cuh Outdated Show resolved Hide resolved
cpp/src_prims/distance/euclidean.cuh Outdated Show resolved Hide resolved
cpp/src_prims/distance/euclidean.cuh Outdated Show resolved Hide resolved
cpp/src_prims/distance/euclidean.cuh Outdated Show resolved Hide resolved
@mdoijade
add review changes - move kernel to base class, move common epilog co…
3ff0707 
…de of l2,cosine to base class, move sqrt case from base class to epilog, make use of assert instead of if
@mdoijade mdoijade requested a review from teju85 March 18, 2021 15:27
@github-actions github-actions bot removed the CMake label Mar 23, 2021
@JohnZed JohnZed added this to PR-WIP in v0.19 Release via automation Mar 24, 2021
v0.19 Release automation moved this from PR-WIP to PR-Reviewer approved Mar 24, 2021
teju85
teju85 approved these changes Mar 24, 2021
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@dantegd dantegd mentioned this pull request Mar 24, 2021
Open
@teju85
Copy link
Member

teju85 commented Mar 25, 2021

@dantegd one of the test_pca_defaults tests are now failing. Any ideas what's happening?

@teju85
Copy link
Member

teju85 commented Mar 25, 2021

rerun tests

@mdoijade
Copy link
Contributor Author

Attaching performance results with prims_benchmark for Distance* cases of this PR vs old code with cutlass for row major inputs.
speedups.xlsx

@teju85
Copy link
Member

teju85 commented Mar 26, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0883026 into rapidsai:branch-0.19 Mar 26, 2021
v0.19 Release automation moved this from PR-Reviewer approved to Done Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teju85 teju85 teju85 approved these changes

Assignees
No one assigned
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
v0.19 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mdoijade @GPUtester @JohnZed @teju85