Replace ML::MetricType with raft::distance::DistanceType #3389

lowener · 2021-01-20T11:07:19Z

Closes #3319.
This PR will replace the distance type from ML::MetricType to raft::distance::DistanceType.

Since Raft DistanceType makes the distinction between the expanded and non-expanded distances in the name, I changed the C++ API to remove the boolean parameter expanded which becomes useless.

…gument

viclafargue

Looks good, just have a few suggestions.

viclafargue · 2021-02-12T18:06:53Z

cpp/include/cuml/neighbors/knn.hpp

-                     MetricType metric = MetricType::METRIC_L2,
-                     float metric_arg = 2.0f, bool expanded = false);
+                     raft::distance::DistanceType metric =
+                       raft::distance::DistanceType::L2Unexpanded,


I think we may want L2SqrtUnexpanded here to have the euclidean distance as default. At least if the results is seen by the end-user and not only used internally. Normally, METRIC_L2 in FAISS provides the euclidean distance before root-squaring. Then post-processing should apply the root-square. @cjnolet probably knows better about this though.

I would leave these in expanded form, actually. It's the most used metric and the difference in performance is pretty huge

viclafargue · 2021-02-12T18:08:41Z

cpp/include/cuml/neighbors/knn_sparse.hpp

-                     ML::MetricType metric = ML::MetricType::METRIC_L2,
-                     float metricArg = 0, bool expanded_form = false);
+                     raft::distance::DistanceType metric =
+                       raft::distance::DistanceType::L2Unexpanded,


Same, L2SqrtUnexpanded might be needed here.

Same here, I would leave this in expanded form.

viclafargue · 2021-02-12T18:10:32Z

cpp/src/knn/knn_api.cpp

@@ -71,7 +71,8 @@ cumlError_t knn_search(const cumlHandle_t handle, float **input, int *sizes,
    try {
      ML::brute_force_knn(*handle_ptr, input_vec, sizes_vec, D, search_items, n,
                          res_I, res_D, k, rowMajorIndex, rowMajorQuery,
-                          (ML::MetricType)metric_type, metric_arg, expanded);
+                          (raft::distance::DistanceType)metric_type,


Do we need to keep making this conversion explicit?

Since we're using the same enum type everywhere now, I think this conversion can be removed.

This conversion from int to DistanceType is needed to not modify the C API of knn_search. I changed it to a static_cast.

viclafargue · 2021-02-12T18:11:53Z

cpp/src/tsne/distances.cuh

@@ -89,7 +89,7 @@ void get_distances(const raft::handle_t &handle,
    k_graph.knn_indices, k_graph.knn_dists, k_graph.n_neighbors,
    handle.get_cusparse_handle(), handle.get_device_allocator(), stream,
    ML::Sparse::DEFAULT_BATCH_SIZE, ML::Sparse::DEFAULT_BATCH_SIZE,
-    ML::MetricType::METRIC_L2);
+    raft::distance::DistanceType::L2Expanded);


I think we need L2SqrtUnexpanded here, unless this distance value doesn't reach user's eye (only used by TSNE internally).

This doesn't reach the users so I think it's okay not to use the sqrt here. I think the expanded form is also good to use here for speed.

viclafargue · 2021-02-12T18:12:30Z

cpp/src/umap/knn_graph/algo.cuh

@@ -91,7 +92,7 @@ void launcher(const raft::handle_t &handle,
    inputsB.n, inputsB.d, out.knn_indices, out.knn_dists, n_neighbors,
    handle.get_cusparse_handle(), d_alloc, stream,
    ML::Sparse::DEFAULT_BATCH_SIZE, ML::Sparse::DEFAULT_BATCH_SIZE,
-    ML::MetricType::METRIC_L2);
+    raft::distance::DistanceType::L2Expanded);


Same, L2SqrtUnexpanded might be needed here.

I would leave all of these in expanded form. The unexpanded is more stable under some conditions but in general it's a better (and faster) starting point.

viclafargue · 2021-02-12T18:36:08Z

cpp/src_prims/selection/knn.cuh

+  if (metric == raft::distance::DistanceType::L2SqrtExpanded ||
+      metric == raft::distance::DistanceType::L2SqrtUnexpanded ||
+      metric == raft::distance::DistanceType::LpUnexpanded) {


Might be wrong, but I think that in this case, only unexpanded forms (that need post-procesing) : L2SqrtUnexpanded and LpUnexpanded should have post-processing. @cjnolet probably knows more about this.

FAISS only supports the expanded form but I believe we're converting both the Expanded and Unexpanded L2 forms into faiss::METRIC_L2 so we'll need to sqrt both of them.

viclafargue · 2021-02-15T11:04:05Z

cpp/src_prims/sparse/selection/knn.cuh

-               ML::MetricType metric_ = ML::MetricType::METRIC_L2,
-               float metricArg_ = 0, bool expanded_form_ = false)
+               raft::distance::DistanceType metric_ =
+                 raft::distance::DistanceType::L2Unexpanded,


Same, L2SqrtUnexpanded might be needed here.

I would revert this to expanded form as well.

viclafargue · 2021-02-15T11:10:24Z

cpp/src_prims/sparse/selection/knn.cuh

-                     ML::MetricType metric = ML::MetricType::METRIC_L2,
-                     float metricArg = 0, bool expanded_form = false) {
+                     raft::distance::DistanceType metric =
+                       raft::distance::DistanceType::L2Unexpanded,


Same, L2SqrtUnexpanded might be needed here.

viclafargue · 2021-02-15T11:15:27Z

python/cuml/neighbors/nearest_neighbors.pyx

        if metric == "euclidean" or metric == "l2":
-            m = MetricType.METRIC_L2
+            m = DistanceType.L2SqrtExpanded


Same, L2SqrtUnexpanded might be needed here.

The Expanded version is preferred for speed

viclafargue · 2021-02-15T11:16:40Z

python/cuml/neighbors/nearest_neighbors.pyx

        elif metric == "cityblock" or metric == "l1"\
                or metric == "manhattan" or metric == 'taxicab':


Can maybe be replaced by elif metric in [..., ...]:.

cjnolet

LGTM!

cjnolet · 2021-02-22T16:16:10Z

cpp/src/knn/knn_api.cpp

@@ -57,6 +57,8 @@ cumlError_t knn_search(const cumlHandle_t handle, float **input, int *sizes,
  cumlError_t status;
  raft::handle_t *handle_ptr;
  std::tie(handle_ptr, status) = ML::handleMap.lookupHandlePointer(handle);
+  raft::distance::DistanceType metric_distance_type =


I like this. The intent becomes more clear.

cjnolet · 2021-02-22T16:21:54Z

cpp/src_prims/sparse/selection/knn.cuh

@@ -434,8 +378,12 @@ class sparse_knn_t {
    dist_config.allocator = allocator;
    dist_config.stream = stream;

-    raft::sparse::distance::pairwiseDistance(batch_dists, dist_config,
-                                             get_pw_metric(), metricArg);
+    if (raft::sparse::distance::supportedDistance.find(metric) ==


I like the introduction of the explicit set for supported distances. For a follow-on / future PR, we might want to consider just using a hash map to map the distance enum type to its “distances_t” implementation, which will allow us to get rid of the switch statement altogether.

cjnolet · 2021-02-22T16:24:37Z

@gpucibot merge

cjnolet · 2021-02-22T18:03:30Z

Adding a reminder / note here that the dense & sparse knn primitives will need to be updated in RAFT before #3476 goes in.

cjnolet · 2021-02-22T20:23:15Z

rerun tests

cjnolet · 2021-02-23T17:43:44Z

@lowener, according to the logs, it looks like there was a gtest failure in KNNTest:

[----------] 1 test from KNNTestF
[ RUN      ] KNNTestF.Fit
/opt/conda/envs/rapids/conda-bld/libcuml_1614025928022/work/cpp/test/prims/knn.cu:103: Failure
Value of: raft::devArrMatch(d_ref_D, d_pred_D, n * n, raft::CompareApprox<float>(1e-3))
  Actual: false (actual=2401 != expected=49 @1)
Expected: true
[  FAILED  ] KNNTestF.Fit (218 ms)

JohnZed · 2021-02-25T17:14:42Z

@gpucibot merge

cjnolet · 2021-03-02T02:18:17Z

rerun tests

codecov-io · 2021-03-02T05:59:53Z

Codecov Report

Merging #3389 (03fd464) into branch-0.19 (39c7262) will increase coverage by 9.02%.
The diff coverage is 73.19%.

@@               Coverage Diff               @@
##           branch-0.19    #3389      +/-   ##
===============================================
+ Coverage        71.77%   80.80%   +9.02%     
===============================================
  Files              212      227      +15     
  Lines            17075    17735     +660     
===============================================
+ Hits             12256    14331    +2075     
+ Misses            4819     3404    -1415

Flag	Coverage Δ
dask	`45.30% <15.38%> (?)`
non-dask	`73.06% <71.79%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...on/cuml/_thirdparty/sklearn/preprocessing/_data.py	`63.24% <ø> (-0.30%)`	⬇️
python/cuml/experimental/explainer/common.py	`88.05% <42.85%> (-4.01%)`	⬇️
python/cuml/neighbors/ann.pyx	`61.62% <61.62%> (ø)`
python/cuml/common/import_utils.py	`59.43% <66.66%> (+3.43%)`	⬆️
python/cuml/experimental/explainer/base.pyx	`67.06% <67.06%> (ø)`
python/cuml/metrics/hinge_loss.pyx	`73.33% <73.33%> (ø)`
python/cuml/dask/common/utils.py	`43.68% <83.33%> (+16.13%)`	⬆️
python/cuml/neighbors/nearest_neighbors.pyx	`92.25% <85.71%> (-0.46%)`	⬇️
...l/_thirdparty/sklearn/preprocessing/_imputation.py	`62.80% <100.00%> (+0.29%)`	⬆️
python/cuml/experimental/explainer/kernel_shap.pyx	`97.75% <100.00%> (+0.48%)`	⬆️
... and 108 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f06d4b...03fd464. Read the comment docs.

mike-wendt · 2021-03-02T15:35:16Z

rerun tests

github-actions bot added Cython / Python Cython or Python issue libcuml labels Jan 20, 2021

lowener changed the title ~~Replace ML::MetricType with raft::distance::DistanceType~~ [WIP] Replace ML::MetricType with raft::distance::DistanceType Jan 20, 2021

lowener added 9 commits January 28, 2021 06:11

Added raft::MetricType on multiple knn functions

962357f

Added raft distance to sparse functions & removed 'expanded forms' ar…

77decf2

…gument

Continued conversion of Cpp code for KNN metrics

9e33273

Conversion of python side of knn

a7e9093

Removed expanded argument of brute_force_knn on python side

134d251

Removed 'Euc' from distance name to follow Raft

234898a

Changed distance naming for prims processing

5261873

Changing distance type in src_prims

050f4d0

Changing tests and bench distancetype naming

c96a8c4

lowener force-pushed the 018_replace_distancetype branch from 6529592 to c96a8c4 Compare January 28, 2021 14:24

lowener added 5 commits January 28, 2021 17:47

Fixing compilation errors with rebase

fcfc8b7

Default L2 metric to Expanded

fc3cc75

Restoring 2 raft distance metrics to knn

50c29c6

Correct logic for unexpanded post-processing

09200e0

Fixed logic for python side NN

af05da6

lowener changed the title ~~[WIP] Replace ML::MetricType with raft::distance::DistanceType~~ Replace ML::MetricType with raft::distance::DistanceType Jan 31, 2021

lowener marked this pull request as ready for review January 31, 2021 14:57

lowener requested review from a team as code owners January 31, 2021 14:57

lowener added 5 commits January 31, 2021 10:11

Merge branch 'branch-0.18' into 018_replace_distancetype

380883c

Update copyrights & fix style

2441139

Fix style

a53ed06

Fix Distancetype call in tsne

521b983

Fix copyright

44dcb80

cjnolet added this to PR-WIP in v0.18 Release via automation Feb 3, 2021

cjnolet added the 3 - Ready for Review Ready for review by team label Feb 3, 2021

Change distancetype selection from python

fa136e8

Fix distance type for umap algorighm

cc95e28

lowener requested a review from cjnolet February 11, 2021 22:15

cjnolet added 3 - Ready for Review Ready for review by team and removed 4 - Waiting on Author Waiting for author to respond to review labels Feb 11, 2021

viclafargue requested changes Feb 15, 2021

View reviewed changes

Changed DistanceType cast in C API

11f668e

github-actions bot added the CUDA/C++ label Feb 19, 2021

Changed default metric from L2Unexpanded to L2Expanded

a419589

cjnolet approved these changes Feb 22, 2021

View reviewed changes

v0.19 Release automation moved this from PR-Needs review to PR-Reviewer approved Feb 22, 2021

cjnolet mentioned this pull request Feb 22, 2021

Single-linkage AgglomerativeClustering & HDBSCAN #3308

Closed

lowener added 3 commits February 24, 2021 08:44

Changed metric for tests according to the new DistanceType

0ff0bbe

Merge branch 'branch-0.19' into 018_replace_distancetype

819991d

Update copyright

6259a0b

cjnolet mentioned this pull request Feb 24, 2021

[REVIEW] Removing FusedL2NN and sparse prims from cuml #3554

Closed

Fixed mistake during merge to branch-0.19

9c1b466

JohnZed added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Feb 25, 2021

Temporarily disabling test_hinge_loss

03fd464

rapids-bot bot merged commit 9fa6e17 into rapidsai:branch-0.19 Mar 2, 2021

v0.19 Release automation moved this from PR-Reviewer approved to Done Mar 2, 2021

lowener deleted the 018_replace_distancetype branch April 8, 2021 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ML::MetricType with raft::distance::DistanceType #3389

Replace ML::MetricType with raft::distance::DistanceType #3389

lowener commented Jan 20, 2021 •

edited

viclafargue left a comment

viclafargue Feb 12, 2021

cjnolet Feb 19, 2021

viclafargue Feb 12, 2021

cjnolet Feb 19, 2021

viclafargue Feb 12, 2021

cjnolet Feb 18, 2021

lowener Feb 19, 2021

viclafargue Feb 12, 2021

cjnolet Feb 16, 2021

viclafargue Feb 12, 2021

cjnolet Feb 19, 2021

viclafargue Feb 12, 2021

cjnolet Feb 16, 2021

viclafargue Feb 15, 2021

cjnolet Feb 19, 2021

viclafargue Feb 15, 2021

cjnolet Feb 19, 2021

viclafargue Feb 15, 2021

lowener Feb 19, 2021

viclafargue Feb 15, 2021

cjnolet left a comment

cjnolet Feb 22, 2021

cjnolet Feb 22, 2021

cjnolet commented Feb 22, 2021

cjnolet commented Feb 22, 2021

cjnolet commented Feb 22, 2021

cjnolet commented Feb 23, 2021

JohnZed commented Feb 25, 2021

cjnolet commented Mar 2, 2021

codecov-io commented Mar 2, 2021

mike-wendt commented Mar 2, 2021

		elif metric == "cityblock" or metric == "l1"\
		or metric == "manhattan" or metric == 'taxicab':

Replace ML::MetricType with raft::distance::DistanceType #3389

Replace ML::MetricType with raft::distance::DistanceType #3389

Conversation

lowener commented Jan 20, 2021 • edited

viclafargue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjnolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjnolet commented Feb 22, 2021

cjnolet commented Feb 22, 2021

cjnolet commented Feb 22, 2021

cjnolet commented Feb 23, 2021

JohnZed commented Feb 25, 2021

cjnolet commented Mar 2, 2021

codecov-io commented Mar 2, 2021

Codecov Report

mike-wendt commented Mar 2, 2021

lowener commented Jan 20, 2021 •

edited