Skip to content

[MRG] Make OPTICS more memory efficient when calling kneighbors#12103

Merged
qinhanmin2014 merged 5 commits intoscikit-learn:masterfrom
TomDLT:optics_chunk
Sep 22, 2018
Merged

[MRG] Make OPTICS more memory efficient when calling kneighbors#12103
qinhanmin2014 merged 5 commits intoscikit-learn:masterfrom
TomDLT:optics_chunk

Conversation

@TomDLT
Copy link
Copy Markdown
Member

@TomDLT TomDLT commented Sep 18, 2018

Fixes #12098

It calls kneighbors on chunks to avoid exceeding working_memory.


# OPTICS helper functions

def _calculate_core_distances_(self, X, nbrs, working_memory=None):
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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