MNT Removes duplicates in neighbors.VALID_METRICS["brute"] #22602
Conversation
sklearn/neighbors/_base.py
Outdated
| "sqeuclidean", | ||
| "yule", | ||
| ] | ||
| brute=list( |
There was a problem hiding this comment.
I think the failures in the CI come from the parametrization over parameters that come from a set. According to pytest-dev/pytest-xdist#598, we should use sorted(set) instead of list(set).
| brute=list( | |
| brute=sorted( |
There was a problem hiding this comment.
@jeremiedbb Thank you for your prompt comment!
I was just about to investigate the CI errors.
I will try.
There was a problem hiding this comment.
@jeremiedbb Ah. you have already fixed it. Thanks!
There was a problem hiding this comment.
Thanks for the PR @i-aki-y !
I left a minor comment, otherwise LGTM
sklearn/neighbors/_base.py
Outdated
| "yule", | ||
| ] | ||
| brute=sorted( | ||
| set(PAIRWISE_DISTANCE_FUNCTIONS.keys()).union( |
There was a problem hiding this comment.
Small nit:
| set(PAIRWISE_DISTANCE_FUNCTIONS.keys()).union( | |
| set(PAIRWISE_DISTANCE_FUNCTIONS).union( |
(Iterating through a dictionary uses the keys by default)
There was a problem hiding this comment.
@thomasjpfan Thank you for your review. I fixed it.
thomasjpfan
changed the title
thomasjpfan
changed the title
…rn#22602) Co-authored-by: akiyuki ishikawa <aki.y.ishikwa@gmail.com> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
…rn#22602) Co-authored-by: akiyuki ishikawa <aki.y.ishikwa@gmail.com> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
3 participants
Reference Issues/PRs
Fixes #22584
What does this implement/fix? Explain your changes.
This PR ensures that the constant list:
sklearn.neighbors.VALID_METRICS["brute"]has no duplicate.Since the
VALID_METRICS["brute"]is defined as a concatenation of different lists, the resulting list can unintentionally contain duplicate values. The PR continue to ensures the resulting list is unique by replacinglist1 + list2withlist(set(list1).union(list2)).Any other comments?
No.