scholar icon indicating copy to clipboard operation
scholar copied to clipboard

Refactor Scholar.Neighbors

Open krstopro opened this issue 10 months ago • 1 comments

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

  • [x] 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 #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.

krstopro avatar Apr 11 '24 11:04 krstopro

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

josevalim avatar Apr 11 '24 11:04 josevalim

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 avatar May 07 '24 10:05 josevalim

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

josevalim avatar May 07 '24 10:05 josevalim

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.

krstopro avatar May 07 '24 11:05 krstopro

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

josevalim avatar May 07 '24 11:05 josevalim

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

josevalim avatar May 14 '24 21:05 josevalim