Skip to content
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

Optionally enforce exact distances #667

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jondo
Copy link
Contributor

@jondo jondo commented May 10, 2021

Since the threshold of 4096 for deciding the distance algorithm is a bit arbitrary and depends on machine speed, this pull request introduces another argument force_exact_distancesas an opposite to the existing force_approximation_algorithm.

@pep8speaks
Copy link

pep8speaks commented May 10, 2021

Hello @jondo! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-10 13:33:14 UTC

@coveralls
Copy link

coveralls commented May 10, 2021

Coverage Status

Coverage decreased (-0.01%) to 88.372% when pulling 431787f on jondo:force_exact_distances into 47b585c on lmcinnes:master.

@jondo jondo force-pushed the force_exact_distances branch from a82f9e6 to 431787f Compare May 10, 2021 13:33
@lmcinnes
Copy link
Owner

This could certainly be useful, but it will be brutally slow (and incredibly memory hungry) for a number of use cases. It might be worth at least using sklearn's NearestNeighbor or KNeighborTransformer classes for the computation rather than computing all pairwise distances (which is fine for small data, but perilously memory expensive for large data).

@jondo
Copy link
Contributor Author

jondo commented May 12, 2021

@lmcinnes , this sounds plausible, but I won't do it now. Feel free to close this request.

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.

4 participants