-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[MRG] Make OPTICS more memory efficient when calling kneighbors #12103
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
Conversation
sklearn/cluster/optics_.py
Outdated
@@ -425,6 +422,42 @@ def fit(self, X, y=None): | |||
|
|||
# OPTICS helper functions | |||
|
|||
def _calculate_core_distances_(self, X, nbrs, working_memory=None): |
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.
As a non-native English speaker, I wonder if "calculate" to is natural or not. It sounds French to my ears and I would have thought that "compute" would be more idiomatic but I would love to get the feedback of a native English speaker.
Any way, it's a private function so it's not necessary to change this.
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.
sklearn/cluster/optics_.py
Outdated
---------- | ||
X : array, shape (n_samples, n_features) | ||
The data. | ||
nbrs : NearestNeighbors instance |
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.
I would rather use "neighbors" instead of "nbrs" that parses as "numbers" in my brains.
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, I'll merge this one for other OPTICS PRs.
Fixes #12098
It calls
kneighbors
on chunks to avoid exceedingworking_memory
.