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

[REVIEW] KL Divergence metric implementation #674

Merged
merged 15 commits into from
Jun 30, 2019
Merged

[REVIEW] KL Divergence metric implementation #674

merged 15 commits into from
Jun 30, 2019

Conversation

venkywonka
Copy link
Contributor

  • KL Divergence implementation and unit tests for the same
  • wrapper exposing implementation to cuML API

Ganesh Venkataramana added 3 commits June 12, 2019 04:23
Merge branch 'branch-0.8' of https://github.com/rapidsai/cuml into fe…
c81dc6e 
…a-ext-kl-divergence
KL Divergence metric CUDA implementation
5d8fb53 
- KL Divergence implementation and gtests for the same
- wrapper implementation exposing the metric to cuML API
updated CHANGELOG.md
bf1bfc0
@venkywonka venkywonka changed the title KL Divergence metric implementation [WIP] KL Divergence metric implementation Jun 12, 2019
@teju85
Copy link
Member

teju85 commented Jun 12, 2019

@venkywonka seems like you have also included the files from the PR #652 as well?

Not sure what's the recommended workflow here.
@cjnolet @dantegd is it better to have a fresh branch in such cases or directly start working on the previous branch?

@venkywonka
Copy link
Contributor Author

@venkywonka seems like you have also included the files from the PR #652 as well?

Not sure what's the recommended workflow here.
@cjnolet @dantegd is it better to have a fresh branch in such cases or directly start working on the previous branch?

ohh... yea. my bad, the fea-ext-kl-divergence branch was created from local repo so had the ARI files too.

@dantegd dantegd added 2 - In Progress Currenty a work in progress CUDA / C++ CUDA issue New Prim For tracking new prims that will be added to our existing collection labels Jun 19, 2019
teju85
teju85 requested changes Jun 19, 2019
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
cpp/src/metrics/metrics.hpp Outdated Show resolved Hide resolved
cpp/src_prims/metrics/klDivergence.h Outdated Show resolved Hide resolved
cpp/src_prims/metrics/klDivergence.h Show resolved Hide resolved
cpp/test/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/test/prims/klDivergence.cu Show resolved Hide resolved
Ganesh Venkataramana added 2 commits June 19, 2019 04:10
review changes
780bd51 
- fixed conflicts in changelog
- removed some commented code
- exposed cuML API wrapper made to support both double and float types
- added CUDA_CHECK(cudaStreamSynchronize(stream));
- refactored CMakeLists prim lists in alphabetical order
some unreflected changes of previous commit
a5e0954
teju85
teju85 requested changes Jun 19, 2019
View reviewed changes
cpp/src/metrics/metrics.hpp Outdated Show resolved Hide resolved
teju85
teju85 requested changes Jun 24, 2019
View reviewed changes
cpp/src/metrics/metrics.hpp Outdated Show resolved Hide resolved
cpp/src/metrics/metrics.hpp Outdated Show resolved Hide resolved
cpp/src_prims/metrics/klDivergence.h Outdated Show resolved Hide resolved
review changes
ba92af6 
- indented comments properly
- avoided redundantly copying the input
teju85
teju85 approved these changes Jun 24, 2019
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 now. @cjnolet Please merge.

@venkywonka venkywonka changed the base branch from branch-0.8 to branch-0.9 June 25, 2019 15:00
teju85
teju85 requested changes Jun 26, 2019
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
teju85
teju85 approved these changes Jun 27, 2019
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.

This PR LGTM, @cjnolet please review and merge.

@venkywonka venkywonka changed the title [WIP] KL Divergence metric implementation [REVIEW] KL Divergence metric implementation Jun 28, 2019
@cjnolet cjnolet added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Jun 28, 2019
@cjnolet
Copy link
Member

cjnolet commented Jun 28, 2019

Will merge once conflicts are resolved

@cjnolet
Copy link
Member

cjnolet commented Jun 30, 2019

@danielhanchen can you use this prim on TSNE?

@cjnolet cjnolet merged commit efe2761 into rapidsai:branch-0.9 Jun 30, 2019
@cjnolet
Copy link
Member

cjnolet commented Aug 21, 2019

#608

@yuqli yuqli mentioned this pull request Oct 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet approved these changes

@teju85 teju85 teju85 approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team CUDA / C++ CUDA issue New Prim For tracking new prims that will be added to our existing collection
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@venkywonka @teju85 @cjnolet @dantegd