-
-
Notifications
You must be signed in to change notification settings - Fork 26k
MAINT Use _validate_params in Nearest Centroid #23874
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
MAINT Use _validate_params in Nearest Centroid #23874
Conversation
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Since this is fixing the bug reported in #23890, could you please add a FIX changelog entry for this estimator targeting 1.2? See Something like: |
Also, please remove the unecessary import reported by the
|
I just realized that the common test is also failing: but I don't understand why. Any idea @jeremiedbb or @glemaitre? |
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.
Probably that the documentation about the metric
param should be updated.
Beware, this will conflict with main
(see merged PR #23806).
@glemaitre we actually need a changelog entry as explained in #23874 (comment). |
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Oh yes I see. @2357juan Could you solve the linting problem and add an entry in the what's new |
You'll also need to modify some tests in
and you can safely delete |
@ogrisel, the issue with the failure happening only at predict time only applies to |
I pushed a more conservative validation of the metric options with only discarding the failing at predict metrics. I think that if we really want to not support all metrics but euclidean and manhattan, we should make a proper deprecation in a separate PR. |
LGTM then. We can open a subsequent PR as suggested by @jeremiedbb |
I'm working on it (if that's OK with you @2357juan) |
Reference Issues/PRs
towards #23462
Ref #23890
What does this implement/fix? Explain your changes.
Added _parameter_constraints class variable to Nearest Centroid.
Added metrics pairwise _VALID_METRICS constraints.
Any other comments?
The left bound of the shrink_threshold of 0 may need some review.