-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MAINT Move DistanceMetric
under metrics
#21177
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 Move DistanceMetric
under metrics
#21177
Conversation
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.
LGTM once the following is taken care of:
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
#cython: cdivision=True | ||
# cython: boundscheck=False | ||
# cython: cdivision=True | ||
# cython: initializedcheck=False |
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.
Note that checks settable via initializedcheck
are activated by default.
I preferred to turned it off here: in practice, it does not change something but it might for new interfaces such as the ones introduced in #20254.
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…-learn#21248) * Remove SplineTransformer from DOCSTRING_IGNORE_LIST * Fix numpydocs from SplineTransformer
…cikit-learn#21277) * Remove SelfTrainingClassifier from DOCSTRING_IGNORE_LIST * Fix numpydocs from SelfTrainingClassifier * Change docstrings to maintain consistency
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
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.
Here is the suggested rephrasing to lighten a bit the docstring while making them more accurate.
…ngly Co-authored-by: Roman Yurchak <rth.yurchak@pm.me> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
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.
Thanks!
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Roman Yurchak <rth.yurchak@pm.me> Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
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.
2 approvals. I think all comments have been resolved.
LGTM for me as well. Let's merge. Thanks @jjerphan !
Reference Issues/PRs
Extracted from #20254.
What does this implement/fix? Explain your changes.
DistanceMetric
was introduced withneighbors.*Tree
. However, it is being used in other algorithms.Also #20254 relies on
DistanceMetric
for its implementation inmetrics
.This adds a deprecation cycle on this public interface.
For consistency, this moves
DistanceMetric
undermetrics
.Any other comments?
Type definitions have also been moved to
utils
are they are being moved in several modules.