Skip to content

[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

Merged
merged 5 commits into from
Sep 22, 2018

Conversation

TomDLT
Copy link
Member

@TomDLT TomDLT commented Sep 18, 2018

Fixes #12098

It calls kneighbors on chunks to avoid exceeding working_memory.

@@ -425,6 +422,42 @@ def fit(self, X, y=None):

# OPTICS helper functions

def _calculate_core_distances_(self, X, nbrs, working_memory=None):
Copy link
Member

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.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

----------
X : array, shape (n_samples, n_features)
The data.
nbrs : NearestNeighbors instance
Copy link
Member

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.

Copy link
Member

@qinhanmin2014 qinhanmin2014 left a 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.

@qinhanmin2014 qinhanmin2014 merged commit 5d30b2d into scikit-learn:master Sep 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants