Allow multi-instances of annoy trees to be loaded #6
Conversation
|
👍 |
|
@yonromai romain this has the change I told you about yesterday: having multiple instances of the annoy tree. |
| import java.util.List; | ||
|
|
||
| /** | ||
| * Annoy interface | ||
| * Modeled after: https://github.com/spotify/annoy/blob/master/annoy/__init__.py, sorta | ||
| */ | ||
| public interface AnnoyIndex { | ||
| public interface AnnoyIndex extends Closeable { |
There was a problem hiding this comment.
Any reason against extending AutoCloseable instead?
|
|
||
| public List<Integer> getNearestByVector(List<Float> vector, int nbNeighbors) { | ||
| validateVecSize(vector); | ||
| return primitiveToBoxed(cppGetNearestByVector(boxedToPrimitive(vector), nbNeighbors)); | ||
| return primitiveToBoxed( | ||
| cppGetNearestByVector(this.cppPtr, boxedToPrimitive(vector), nbNeighbors)); |
There was a problem hiding this comment.
why using this. everywhere? cppPtr is a member field
| @@ -94,7 +94,6 @@ public void euclideanDistanceTest() { | |||
| assertEquals(annoyIndex.getDistance(0, 1), expectedEuclideanDistance, EPS); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Do you think it would be useful to add a test for multiple trees?
|
Thanks a lot for your PR! |
|
I see what you mean. In the current state, since you are always reassigning the static annoy_tree with the newly loaded one the older tress don't get destructed. |
|
Updated the PR with all the changes. The test fails on master but passes on this branch! |
We are passing the pointer for the tree so it can be shared between cpp and java. Now, the trees have to be explicitly closed so there is no build up of memory on the cpp side.
Next steps: