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

Feature/upstream update #26

Merged
merged 9 commits into from Dec 17, 2017

Conversation

Projects
None yet
2 participants
@eddelbuettel
Owner

eddelbuettel commented Dec 17, 2017

Also adds experimental Hamming distance support (closes #25)

@eddelbuettel eddelbuettel merged commit cf262df into master Dec 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@eddelbuettel eddelbuettel deleted the feature/upstream_update branch Dec 17, 2017

@erikbern

This comment has been minimized.

erikbern commented Dec 17, 2017

Just out of curiosity why do you include the full source code distribution of annoy here? Can you use a submodule or something similar?

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Dec 17, 2017

Couple of reasons, in random order:

  • I never really got comfortable with submodules
    • I see them used only sparingly in the R world
    • They seem a little out of fashion
  • Having a degree of freedom allows me to add additonal defines etc
    • That has come in handy eg when I carried over CNPy for my RcppCNPy
    • R is older than Python and has fewer base types (ie natively only int in 32 bit and double in 64
    • So the mapping sucks
  • I'm lazy, and Annoy is small enough so that I can just catch up once every few months
  • I don't need all your files (ie can skip Lua, Go, ...)
@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Dec 17, 2017

Oh, and while I have you here: should Hamming work? I got weird results when I carried the unit test over and "parked" two of the tests. Will try to look more closely another day.

@erikbern

This comment has been minimized.

erikbern commented Dec 17, 2017

thanks for the explanation!

Hamming should work, but it's admittedly not as thoroughly tested as the other distance functions... so I wouldn't be shocked if there's some wacky errors somewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment