-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add reset for vector #237
Add reset for vector #237
Conversation
Adding posibility to reset vector value by updating it with null Signed-off-by: Martin Gaievski <[email protected]>
…case test based on similarity score Signed-off-by: Martin Gaievski <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #237 +/- ##
============================================
+ Coverage 83.21% 83.22% +0.01%
- Complexity 864 865 +1
============================================
Files 123 123
Lines 3777 3780 +3
Branches 358 359 +1
============================================
+ Hits 3143 3146 +3
+ Misses 474 473 -1
- Partials 160 161 +1
Continue to review full report at Codecov.
|
Signed-off-by: Martin Gaievski <[email protected]>
… has been set to null Signed-off-by: Martin Gaievski <[email protected]>
Added a bit more tests after discussing with Jack offline, scenario added: after vector has been reset to null we assign some value to vector again. After that document should be discoverable by search and when doc is retrieved the vector value should be updated |
//retrieving document by id | ||
final Map<String, Object> knnDocMap = getKnnDoc(INDEX_NAME, docOneId); | ||
final Float[] vectorInDocument = getVectorFromDocument(knnDocMap, FIELD_NAME); | ||
assertEquals(vectorForDocumentOne.length, vectorInDocument.length); | ||
assertArrayEquals(vectorForDocumentOne, vectorInDocument); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the purpose of this assertion? I think the test can remove this to simplify the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, we can assert documents fields after the update only, this particular assertion is redundant. I'll remove it in next commit
addKnnDoc(INDEX_NAME, docTwoId, FIELD_NAME, vectorForDocumentTwo); | ||
|
||
//checking that one of two documents in retrievable based on closer similarity to query vector | ||
int k = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just make k = 2 and then:
- Make sure 2 docs are returned by the k-NN query
- Update doc 2 to null
- Assert the value returned from retrieving doc is null
- Make sure 1 doc is returned by the k-NN query when k = 2 (doc id 1)
- Update doc 2 back to the vector
- Make sure retrieving doc 2 gives the correct vector
- Make sure 2 docs are returned by the k-NN query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, changing test to use k equals 2 instead of 1 for knn searches
Signed-off-by: Martin Gaievski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks!
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Add ability to reset vector value by using 'null'
Description
Adding possibility to unset vector's value by updating vector field with 'null' value
Issues Resolved
#51
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.