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

K-NN Regressor #268

Merged
merged 6 commits into from
May 16, 2024
Merged

K-NN Regressor #268

merged 6 commits into from
May 16, 2024

Conversation

krstopro
Copy link
Member

@krstopro krstopro commented May 15, 2024

Implements k-NN regressor model and removes previous implementation of k-NN.
As usual there are few things I am not sure about:

  • There is a lot of code repeated from KNNClassifier (e.g. deftransformp compute_knn). Was thinking pulling it out of both modules and putting it somewhere else, but that might make the code harder to understand.
  • Labels y can be both rank-1 and rank-2. I think this is slightly different than usual, where tensors that can contain more than one feature are always rank-2. I would suggest allowing y to be only of rank 2.

Closes #255.

@krstopro
Copy link
Member Author

krstopro commented May 15, 2024

  • Labels y can be both rank-1 and rank-2. I think this is slightly different than usual, where tensors that can contain more than one feature are always rank-2. I would suggest allowing y to be only of rank 2.

I would prefer that we all agree on this before merging the pull request.

@josevalim
Copy link
Contributor

I don't have an opinion. But it is always easier to relax the constraints later (i.e. allow only rank 2 for now and potentially also rank 1 later). @msluszniak do you have any thoughts?

@msluszniak
Copy link
Contributor

Yes, I agree that for now we can accept only 2-dim labels

@krstopro
Copy link
Member Author

Alright, will push the code with changes soon. :)

@krstopro krstopro merged commit 74ed5fe into elixir-nx:main May 16, 2024
2 checks passed
@josevalim
Copy link
Contributor

Beautiful, thank you @krstopro!

Once @msluszniak updates both KNN guides we will ship a new version!!!

@krstopro krstopro deleted the knn-regressor branch July 31, 2024 17:45
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.

Refactor Scholar.Neighbors
3 participants