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

Allow KNNIndex classes to take **kwargs arguments and pass them to KNN library #256

Open
dkobak opened this issue Apr 10, 2024 · 2 comments · May be fixed by #265
Open

Allow KNNIndex classes to take **kwargs arguments and pass them to KNN library #256

dkobak opened this issue Apr 10, 2024 · 2 comments · May be fixed by #265

Comments

@dkobak
Copy link
Contributor

dkobak commented Apr 10, 2024

See title. We discussed it at Dagstuhl. Should be useful for PyNNdescent, but could potentially be useful for Annoy/HNSW/etc at well.

We could potentially allow Affinity classes to take **kwargs as well and pass them to the KNNIndex. Here I'm less sure if that's a good design idea.

@pavlin-policar
Copy link
Owner

Yes, we definitely need to add that! IMO, a general **kwargs would be messy, and something like like a knn_kwargs argument would be better. Scikit-learn also does something similar in their KNN classes, but they call it "params". I prefer "kwargs". What do you think?

@dkobak
Copy link
Contributor Author

dkobak commented Apr 12, 2024

Yes, sounds good!

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 a pull request may close this issue.

2 participants