Fix UMAP and simplicial set functions metric #5490
Conversation
viclafargue
force-pushed
the
fix-umap-metric
branch
from
b8c3d6b
to
0bd5c77
Compare
viclafargue
changed the title
|
@viclafargue While this fix changes the behavior of the estimator class, I would consider the previous one broken and we are now moving towards the intended behavior and thus would not consider this a breaking change. What do you think? |
viclafargue
added
non-breaking
cpp/src/umap/knn_graph/algo.cuh
Outdated
| @@ -62,6 +62,7 @@ inline void launcher(const raft::handle_t& handle, | |||
| ptrs[0] = inputsA.X; | |||
| sizes[0] = inputsA.n; | |||
|
|
|||
| std::vector<int64_t>* translations = nullptr; | |||
There was a problem hiding this comment.
Any reason to introduce a temporary for this?
There was a problem hiding this comment.
The function template won't be instantiated while providing a nullptr directly unless a cast is used it seems. I just switched it for a cast.
| "correlation": DistanceType.CorrelationExpanded, | ||
| "hellinger": DistanceType.HellingerExpanded, | ||
| "hamming": DistanceType.HammingUnexpanded, | ||
| "jaccard": DistanceType.JaccardExpanded, |
There was a problem hiding this comment.
Jaccard is supported in the sparse distances- is there any reason we're not separating the sparse from dense supported metrics? I can't see why we'd want to remove jaccard from being executed on sparse metrics just because it's not yet provided for dense.
There was a problem hiding this comment.
@cjnolet This should be addressed now.
There was a problem hiding this comment.
My bad, didn't saw it was used in the sparse case. Thanks for fixing this @csadorf.
And add note about jaccard only supported for sparse inputs.
| "jaccard", | ||
| "hamming", | ||
| "canberra", | ||
| ("l2", True), |
There was a problem hiding this comment.
We should only have to do the mappings once- if you use strings in SPARSE_SUPPORTED_METRICS and DENSE_SUPPORTED_METRICS then you can literally just use the union of the two here here instead of having to list them out at all.
There was a problem hiding this comment.
Pulling in those lists into the test code would be counter-productive IMO since it correlates implementation and test expectation which means that it becomes harder to detect breaking changes.
|
/merge |
Answers #5422