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

Attempt at adding cosine similarity metric #28

Merged
merged 2 commits into from
Jul 18, 2019
Merged

Conversation

jlevy44
Copy link
Contributor

@jlevy44 jlevy44 commented Jul 10, 2019

Hopefully I'm not too far off. Let's merge when we're both feeling confident about the changes! Thanks!

@jlevy44
Copy link
Contributor Author

jlevy44 commented Jul 10, 2019

#26

I did not test this implementation. Been swamped with some other works, but I can test when the code has been shaped a bit more.

@rusty1s
Copy link
Owner

rusty1s commented Jul 11, 2019

Looks good so far :)

I suggest the following changes:

  • dotProduct and calc_norm should actually take in scalar_t values using templates. And maybe rename them to dot and norm?
  • You said that the sklearn implementation is slower, so I think we should branch here dependent on which metric is used.
  • sklearn needs to be a dependency in setup.py.

@jlevy44
Copy link
Contributor Author

jlevy44 commented Jul 13, 2019

Ok, just updated a bit, still need your help to test and finish. Thanks!

@rusty1s
Copy link
Owner

rusty1s commented Jul 14, 2019

Great, will try to merge tomorrow.

@jlevy44
Copy link
Contributor Author

jlevy44 commented Jul 17, 2019

Awesome let me know how it goes.

@rusty1s rusty1s merged commit 9947d77 into rusty1s:master Jul 18, 2019
@rusty1s
Copy link
Owner

rusty1s commented Jul 18, 2019

Well, I cleaned up the code. It was kinda buggy, didn't you test it? In addition, there is no sklearn kNN implementation for the cosine distance, so currently I raise a NotImplementedError for the cpu 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 this pull request may close these issues.

None yet

2 participants