Closed julsal closed 7 years ago
Hi @a1k0n , thanks for your nice suggestions. I'll incorporate them along with some unit tests.
nice!
it's done!
pinging @yonromai @eshvk
simplification done! I'm happy to contribute :)
I wrote this originally but no longer work at Spotify or I would merge it. :) @erikbern do you still have access?
@a1k0n you don't have merge access?
i have... i can merge if you want me to
@erikbern please do
@erikbern, thanks for merge the PR. Could you also publish the new version in Maven Central? It will help me so much.
@yonromai Romain, could you bump the version and mvn publish?
as a side note it seems a bit unfortunate that annoy-java now has its own file format
nvm misunderstood code
What do you mean? It still loads regular python annoy indexes... and regular python annoy supports >2GB indexes.
(Sorry for the delay, I was on vacation, away from cell towers) @julsal Thanks a lot for you contribution! Did you measure the performance impact of the change? I remember I did a similar change a while back and got a >2x slowdown in NN lookup time, which led to me not merging the change. But totally possible my code was crap.
@yonromai unfortunately, I didn't make any performance test. In the case you do it, I would be interested in the results.
Any chance we'll see an update of the Maven Repo with this PR's changes?
hey @thomas4g we're actually deprecating this version of annoy-java and working on open sourcing a JNI based version. Will keep you posted.
@yonromai ahh, awesome! I was actually going to try my hand at something like that in my spare time - if any development support is needed once it's open-sourced, I'd love to help out!
solving the restriction described in #1.