Add support for multiple spaces #457
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HammadB
commented
May 4, 2023
| @@ -128,7 +132,7 @@ def add(self, ids, embeddings, update=False): | |||
|
|
|||
| labels = [] | |||
| for id in ids: | |||
| if id in self._id_to_label: | |||
| if hexid(id) in self._id_to_label: | |||
HammadB
commented
May 4, 2023
| @@ -141,7 +145,9 @@ def add(self, ids, embeddings, update=False): | |||
| labels.append(next_label) | |||
There was a problem hiding this comment.
Is weird that sometimes we store the UUID vs the id but leaving this for now to minimize the bugs i might introduce.
HammadB
commented
May 4, 2023
| @@ -71,7 +75,7 @@ class Hnswlib(Index): | |||
| _index_metadata: IndexMetadata | |||
| _params: HnswParams | |||
| _id_to_label: Dict[str, int] | |||
| _label_to_id: Dict[int, str] | |||
| _label_to_id: Dict[int, UUID] | |||
There was a problem hiding this comment.
See below comment on how its odd that we store the string and the uuid in opposite direction. Updating the typing.
HammadB
commented
May 4, 2023
| @@ -172,7 +176,7 @@ def persist_generated_data_with_old_version( | |||
| coll = api.create_collection( | |||
| name=collection_strategy.name, | |||
| metadata=collection_strategy.metadata, | |||
| embedding_function=collection_strategy.embedding_function, | |||
| embedding_function=lambda x: None, | |||
There was a problem hiding this comment.
This makes the tests faster because we pass none across the process boundary because we can't pickle a lambda
atroyn
approved these changes
May 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Add support for multiple distance metrics in tests.
In the process of writing this test I discovered a bug with our implementation of update that was revealed by the inner product space. Since the inner product is not a true metric, a point may not be a neighbor to itself. Our update code was strictly appending to the index due to the a bug with how we manage string UUID vs UUID objects. In l2 and cosine spaces, this usually was fine in the eyes of tests since the results returned were correct with the updated data. But IP exacerbated the issue by making the results not always be the same point.
Test plan
These are tests!
Documentation Changes
None required!