-
Notifications
You must be signed in to change notification settings - Fork 532
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
[REVIEW] Move DistanceType enum to RAFT #2918
Conversation
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
Linked RAFT PR: rapidsai/raft#73 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but can you target at 0.17 branch instead?
Yes of course! I'd actually meant to ask about what namespace made sense when I posted this. I'll update tomorrow. |
Oh and yep, I'll update to target 0.17 as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look mostly good to me, but let's run clang-tidy on the RAFT side of changes and cascade any naming convention diffs down here - I'll add the same comment there
Looks like conda timed out in solving the environment. Retrying... |
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-0.17 #2918 +/- ##
============================================
Coverage 59.23% 59.23%
============================================
Files 142 142
Lines 8966 8966
============================================
Hits 5311 5311
Misses 3655 3655 Continue to review full report at Codecov.
|
@divyegala This finally worked its way through CI (hooray!). Would you mind giving it one final review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Took care of merge conflicts; no other changes |
Merge conflicts were causing too many problems to easily track down. Closing in favor of #3141. |
Update all references from the DistanceType enum defined in cuML to the equivalent defined in RAFT. This change facilitates future re-use of this enum across Cython and prims code.