FIX Add "sqeuclidean" to valid metrics for PairwiseDistancesReduction
#24771
Conversation
This metric has been forgotten in the past.
| @@ -73,7 +73,7 @@ def valid_metrics(cls) -> List[str]: | |||
| "hamming", | |||
| *BOOL_METRICS, | |||
| } | |||
| return sorted(set(METRIC_MAPPING.keys()) - excluded) | |||
| return sorted(({"sqeuclidean"} | set(METRIC_MAPPING.keys())) - excluded) | |||
There was a problem hiding this comment.
Should we use _VALID_METRICS instead ?
scikit-learn/sklearn/metrics/pairwise.py
Line 1622 in 27d3899
There was a problem hiding this comment.
_VALID_METRICS and METRIC_MAPPING.keys() intersect but none isn't included in the other.
METRIC_MAPPING.keys() is the list of DistanceMetric within scikit-learn while _VALID_METRICS has other distance metrics (like "sqeuclidean" that we conceptually support without any DistanceMetric via the Euclidean* specialisation).
Hence, I think f9beef1 can be reverted and we can implement new DistanceMetric in the future (or have our be in common with SciPy's).
What do you think?
There was a problem hiding this comment.
Here are the metrics in VALID_METRICS that are not in METRIC_MAPPING:
"correlation",
"cosine",
"sqeuclidean",
"yule",
"nan_euclidean",
and here are the ones that are in METRIC_MAPPING but not in VALID_METRICS:
'p'
'infinity'
'pyfunc'
I agree, let's keep using METRIC_MAPPING for now. At some point it would nice to have more consistency throughout the pairwise module, but let's leave that for later :)
Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
This reverts commit f9beef1.
|
The error observed on the CI should be fixed by the NaN guard introduced in #24542, another small PR I am currently assessing any performance regression of. |
Reference Issues/PRs
Relates to #24556.
What does this implement/fix? Explain your changes.
This adds
"sqeuclidean"to valid metrics forPairwiseDistancesReductionwhich simplies has been forgotten for inclusion in the past.Any other comments?
Originally observed by @Vincent-Maladiere in #24556.
Any other distance metrics that I forgot? 🙃