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

Search index is optional for precomputed_knn #909

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

jlmelville
Copy link
Collaborator

This is a fix for #848 in the sense that it allows passing only indices and distances to as the precomputed_knn parameter (i.e. no pynndescent object required). Things to note:

  • You will get a warning that you can't transform new data if you do this.
  • If you then try to transform new data via the transform method, a NotImplementedError will be raised.
  • If you have small data and force_approximation_algorithm=False you will not see the new warning, because the knn will not be used. Setting force_approximation_algorithm=True avoids this situation so this new feature works for big and small data.
  • docstring for precomputed_knn parameter on the UMAP constructor has been updated.

Tests successes/warns/failures were the same before and after this addition (there is one failure currently which was failing on upstream-master for me before I added any code). I have added a new test to exercise these various cases.

@lmcinnes
Copy link
Owner

lmcinnes commented Sep 6, 2022

Thanks, this looks like a nice simple way to resolve the issue.

@lmcinnes lmcinnes merged commit e7ebe29 into lmcinnes:master Sep 6, 2022
This pull request was closed.
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.

2 participants