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] MNMG KNN Classifier & Regressor #2211

Merged
merged 53 commits into from
Jul 24, 2020
Merged

[REVIEW] MNMG KNN Classifier & Regressor #2211

merged 53 commits into from
Jul 24, 2020

Conversation

viclafargue
Copy link
Contributor

@viclafargue viclafargue commented May 7, 2020 •
edited
Loading

Was linked to cumlprims MR #37, corresponding code has been transfered in this PR.

@viclafargue viclafargue requested review from a team as code owners May 7, 2020 09:49
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@dantegd dantegd added the 2 - In Progress Currenty a work in progress label May 8, 2020
@viclafargue viclafargue changed the title [WIP] MNMG KNN Classifier & Regressor [REVIEW] MNMG KNN Classifier & Regressor May 18, 2020
@viclafargue viclafargue added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels May 18, 2020
cjnolet
cjnolet requested changes May 18, 2020
View reviewed changes
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.

This is a great start. I think there's a lot of opportunity for reuse here and following the class hierarchy of the single-GPU implementations should be a big help there. I think it's a good idea to knock these out in this PR as it will make the kneighbors regressor much easier to build.

cpp/src_prims/selection/knn.h Outdated Show resolved Hide resolved
cpp/src_prims/selection/knn.h Outdated Show resolved Hide resolved
cpp/src_prims/selection/knn.h Outdated Show resolved Hide resolved
python/cuml/dask/neighbors/kneighbors_classifier.pyx Outdated Show resolved Hide resolved
python/cuml/dask/neighbors/kneighbors_classifier.pyx Outdated Show resolved Hide resolved
python/cuml/dask/neighbors/kneighbors_classifier.pyx Outdated Show resolved Hide resolved
python/cuml/dask/neighbors/kneighbors_classifier.pyx Outdated Show resolved Hide resolved
python/cuml/test/dask/test_kneighbors_classifier.py Outdated Show resolved Hide resolved
python/cuml/dask/neighbors/kneighbors_classifier.pyx Outdated Show resolved Hide resolved
python/cuml/test/dask/test_kneighbors_classifier.py Outdated Show resolved Hide resolved
@cjnolet cjnolet added 4 - Waiting on Author Waiting for author to respond to review CUDA / C++ CUDA issue Cython / Python Cython or Python issue New Algorithm For tracking new algorithms that will be added to our existing collection and removed 3 - Ready for Review Ready for review by team labels May 19, 2020
@viclafargue viclafargue changed the title [REVIEW] MNMG KNN Classifier & Regressor [WIP] MNMG KNN Classifier & Regressor May 20, 2020
@viclafargue viclafargue requested a review from a team as a code owner July 8, 2020 11:09
@viclafargue
Copy link
Contributor Author

Seems like there was a cuDF update that causes some tests to fail.

@dantegd
Copy link
Member

dantegd commented Jul 17, 2020

rerun tests

1 similar comment
@viclafargue
Copy link
Contributor Author

rerun tests

cjnolet
cjnolet approved these changes Jul 24, 2020
View reviewed changes
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.

LGTM! It's great seeing all this code outside of cumlprims.

@cjnolet cjnolet added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Jul 24, 2020
@cjnolet cjnolet merged commit cf6d1c0 into rapidsai:branch-0.15 Jul 24, 2020
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

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge CUDA / C++ CUDA issue Cython / Python Cython or Python issue New Algorithm For tracking new algorithms 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
@viclafargue @GPUtester @dantegd @cjnolet