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.

Sign up for GitHub

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

Jump to bottom

Update NNDescent #245

Merged
merged 10 commits into from
Apr 5, 2024
Merged

Update NNDescent #245

merged 10 commits into from
Apr 5, 2024

Conversation

msluszniak
Copy link
Contributor

@msluszniak msluszniak commented Mar 25, 2024 •
edited
Loading

Closes #241

@msluszniak msluszniak changed the title Update nn descent Update NNDescent Mar 25, 2024
@krstopro
Copy link
Member

krstopro commented Mar 25, 2024 •
edited
Loading

I should have a closer look, but looks great so far.

One thing that might need to be addressed (I forgot this when the first pull request was submitted) is the following. I don't think the current implementation of random projection forest works with any metric other than (squared) Euclidean. We might wanna raise an error or warning if metric is set to something else and tree_init? is true.

@msluszniak
Copy link
Contributor Author

I should have a closer look, but looks great so far.

One thing that might need to be addressed (I forgot this when the first pull request was submitted) is the following. I don't think the current implementation of random projection forest works with any metric other than (squared) Euclidean. We might wanna raise an error or warning if metric is set to something else and tree_init? is true.

Good call, I'll change this to prevent problematic cases :)

krstopro
krstopro reviewed Mar 26, 2024
View reviewed changes
Copy link
Member

@krstopro krstopro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion regarding the error raised.

msluszniak and others added 4 commits March 28, 2024 14:02
@msluszniak @krstopro
Update lib/scholar/neighbors/nn_descent.ex
177c650 
Co-authored-by: Krsto Proroković <krstopro@yahoo.com>
@msluszniak @josevalim
Update lib/scholar/neighbors/nn_descent.ex
ae0f98c 
Co-authored-by: José Valim <jose.valim@gmail.com>
@msluszniak
Decrease tolerance
ca38a70
@msluszniak
Ensure that all initial nearest neighbors are initialized and fix tes…
5c849a4 
…ts according to the implemented changes
@msluszniak msluszniak merged commit 0619dc5 into elixir-nx:main Apr 5, 2024
2 checks passed
@msluszniak msluszniak deleted the update_nn_descent branch April 5, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krstopro krstopro krstopro left review comments

@josevalim josevalim josevalim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve storing updates in NNDescent algorithm
3 participants
@msluszniak @krstopro @josevalim