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

Refactor Scholar.Neighbors #255

Closed
4 tasks done
krstopro opened this issue Apr 11, 2024 · 6 comments · Fixed by #268
Closed
4 tasks done

Refactor Scholar.Neighbors #255

krstopro opened this issue Apr 11, 2024 · 6 comments · Fixed by #268

Comments

@krstopro
Copy link
Member

krstopro commented Apr 11, 2024

As argued in #254, we need to update Scholar.Neighbors in the following way:

  • Add BruteKNN module that implements brute-force k-NN search. Its predict function has to return {neighbor_indices, neighbor_distances}. Make batch_size an option (as in Batch k-NN #253).
  • Implement KNNClassifier.
  • Implement KNNRegressor.
  • Deprecate Scholar.Neighbors.KNearestNeighbors.

KNNClassifier and KNNRegressor have to take k-NN algorithm and metric options as atoms, but also allow for custom k-NN algorithms and metrics to be passed as modules and functions, respectively.

@josevalim
Copy link
Contributor

We don't need to deprecate, we can just rename the module. It should be easy for anyone to just rename it accordingly. :)

@krstopro krstopro mentioned this issue Apr 13, 2024
@josevalim
Copy link
Contributor

Hi @krstopro, I want to release a new version soon. If we plant to remove Scholar.Neighbors.KNearestNeighbors, it would be better to do it before, so we update the docs accordingly?

@josevalim
Copy link
Contributor

We would also need to mirror those decisions on top of RadiusNearestNeighbors.

@krstopro
Copy link
Member Author

krstopro commented May 7, 2024

Hi @krstopro, I want to release a new version soon. If we plant to remove Scholar.Neighbors.KNearestNeighbors, it would be better to do it before, so we update the docs accordingly?

@josevalim Agreed. I will try to implement the stuff from this issue by the end of the week if that's fine.

We would also need to mirror those decisions on top of RadiusNearestNeighbors.

Correct, shouldn't be hard.

@josevalim
Copy link
Contributor

Perfect. KNN is the most important, Radius can be done later. :)

@josevalim
Copy link
Contributor

Thank you @krstopro! I will work on #265 and @msluszniak will update the notebooks. :) Once KNNRegression is ready, we can release a new version!

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