Splitted RadiusNearestNeighbors module into RNNClassifier and RNNRegressor

[norm4nn]

Following up on #266 - splitted RadiusNearestNeighbors module intoto RNNClassifier and RNNRegressor modules

[josevalim]

Thank you, it looks good to me as a first step! Next we need to look at the failures and tests. :)

[krstopro]

Hi @norm4nn and sorry for the rather delayed review! This looks OK so far, though I am not sure about the approach.

More precisely, I am not sure if we should be having separate modules for Radius Nearest Neighbor Classifier and Regressor or if we should just add the radius parameter to k-NN Classifier and Regressor. I know the issue says to split the existing module into two, but now I am hesitant about that. @josevalim any thoughts on this?

[josevalim]

How many of the existing options in knn classifier play well with the radius one? My understanding is that it is only a subset of them and that the algorithms are different?

[krstopro]

How many of the existing options in knn classifier play well with the radius one? My understanding is that it is only a subset of them and that the algorithms are different?

Indeed, the algorithms are different:

• k-NN makes a prediction based on k-nearest neighbors
• radius nearest neighbors makes a prediction based on all neighbors within a certain radius

The only way to implement the latter using Nx is to look at all the distances and take only those points that are within the radius. Therefore, this works only with the brute-force algorithm.

I guess having separate modules is fine, but they should not have the algorithm option.

[josevalim]

@norm4nn we can go ahead, but I think we need to fix DBSCAN suite, it may rely on the previous models. Can you please take a look?

[krstopro]

@norm4nn we can go ahead, but I think we need to fix DBSCAN suite, it may rely on the previous models. Can you please take a look?

Wrong PR?

[josevalim]

I believe it is the right PR. CI is failing because tests (DBSCAN?) cannot find Scholar.Neighbors.RadiusNearestNeighbors. :)

[krstopro]

I believe it is the right PR. CI is failing because tests (DBSCAN?) cannot find Scholar.Neighbors.RadiusNearestNeighbors. :)

Oh, sorry! The reason I asked is because DBSCAN is mentioned in the other PR.

[norm4nn]

Sure, I will look on this

[josevalim]

There are tiny syntax errors on the doctests and we should be good to go!

[josevalim]

💚 💙 💜 💛 ❤️

[josevalim]

@norm4nn btw, i pushed a better fix for doctests here: 4dec8ba :) what you did basically skipped them from comparing results :)

[krstopro]

I would suggest changing the module names (and maybe file names as well) to RadiusNeighborsClassifier and RadiusNeighborsRegressor just to avoid the confusion with recurrent neural networks (RNN usually stands for recurrent neural networks in machine learning context).

[josevalim]

Good point, I went with RadiusNN for now :)