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] Fix for OPG KNN Classifier & Regressor #2844

Merged
merged 13 commits into from
Oct 6, 2020
Merged

[REVIEW] Fix for OPG KNN Classifier & Regressor #2844

merged 13 commits into from
Oct 6, 2020

Conversation

viclafargue
Copy link
Contributor

No description provided.

@viclafargue viclafargue requested review from a team as code owners September 18, 2020 17:42
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@viclafargue viclafargue changed the title [WIP] Fix for OPG KNN Classifier & Regressor [REVIEW] Fix for OPG KNN Classifier & Regressor Sep 22, 2020
cjnolet
cjnolet requested changes Sep 23, 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.

Overall, I think the MG KNN code is need of some general cleanup to make it easier to read and maintain. I'm largely to blame for this because of the behemoth methods that I had contributed originally. It took awhile to go through this code and I still have a few questions (inline). I'm also still reading through your earlier response to see if there might be a more parallel way to combine the outputs. I wanted to share the review in the meantime, since burndown is looming.

cpp/src/knn/knn_opg_common.cu Show resolved Hide resolved
cpp/src_prims/selection/knn.cuh Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Outdated Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Outdated Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Outdated Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member

cjnolet commented Sep 24, 2020

For some reason, Github is not allowing me to further comment on the following conversation so I'm leaving it in a comment here.

Let's say we have two targets and two workers containing index partitions(s), one of them has a query partition. Let's take this latter worker as example. Before exchange_results, it stores labels as follows : output1_of_rank1 | output2_of_rank1, after exchange_results it stores labels this way : output1_of_rank1 | output1_of_rank2 | output2_of_rank1 | output2_of_rank2. Hope it anwsers your question.

This was not immediately obvious to me just by looking through the code. Can you add this to a comment in the code?

cjnolet
cjnolet previously requested changes Sep 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.

It's looking much better. Thank you for answering my questions! Mostly, I would like to make sure your summaries & descriptions are captured with the actual code both for us and future developers.

cpp/src_prims/selection/knn.cuh Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Outdated Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Outdated Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Outdated Show resolved Hide resolved
cpp/src/knn/knn_opg_common.cu Show resolved Hide resolved
@viclafargue viclafargue mentioned this pull request Sep 24, 2020
Open
@viclafargue
Copy link
Contributor Author

There seems to be some imprecisions on distances on specific configurations (CUDA version / GPU architecture). On a single Tesla V100, I could only reproduce the bug with CUDA 10.1. The bug is probably not related to OPG KNN itself.

@dantegd dantegd added the 4 - Waiting on Reviewer Waiting for reviewer to review or respond label Sep 29, 2020
JohnZed
JohnZed reviewed Oct 5, 2020
View reviewed changes
python/cuml/dask/neighbors/kneighbors_classifier.py
if self.data_handler.datatype == 'cupy':
preds, _, _ = self.predict(X, convert_dtype=convert_dtype)
diff = (preds == y)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "matched" or something similar would be a better name.

JohnZed
JohnZed approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Looks good. Per Corey's suggestions, I agree there is room to simplify the code a bit more, but this looks like a nice improvement that will help in 0.16.

@JohnZed JohnZed 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 Oct 5, 2020
@JohnZed JohnZed dismissed cjnolet’s stale review October 6, 2020 05:03

Comments addressed

@JohnZed JohnZed merged commit 726096a into rapidsai:branch-0.16 Oct 6, 2020
@viclafargue viclafargue mentioned this pull request Oct 13, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnZed JohnZed JohnZed approved these changes

@cjnolet cjnolet cjnolet left review comments

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@viclafargue @GPUtester @cjnolet @JohnZed @dantegd