-
Notifications
You must be signed in to change notification settings - Fork 56
Add support for L1 distance in AKNN, custom scoring and painless scripting #310
Conversation
Codecov Report
@@ Coverage Diff @@
## master #310 +/- ##
============================================
+ Coverage 80.51% 80.61% +0.10%
- Complexity 388 392 +4
============================================
Files 62 62
Lines 1468 1486 +18
Branches 130 133 +3
============================================
+ Hits 1182 1198 +16
- Misses 239 240 +1
- Partials 47 48 +1
Continue to review full report at Codecov.
|
Hi @Elbek-Khoshimjonov, thanks for the PR! I will review this week, |
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.
Hi @Elbek-Khoshimjonov, overall this change looks great! A couple minor comments:
- Could you add L1 below here: https://github.com/opendistro-for-elasticsearch/k-NN/blob/master/src/main/java/com/amazon/opendistroforelasticsearch/knn/index/util/KNNConstants.java#L24
- Could you add L1 here: https://github.com/opendistro-for-elasticsearch/k-NN/blob/master/src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringSpaceFactory.java#L32
- Could you add a test similar to this one? https://github.com/opendistro-for-elasticsearch/k-NN/blob/master/src/test/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScriptScoringIT.java#L48. This should fix test coverage failure.
Done! |
src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java
Show resolved
Hide resolved
can you add the corresponding method to below file like l2Squared? Can you also corresponding tests here? |
Done. |
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 for making this change!
LGTM! Thanks for making changes! |
Feature request from this issue
nmslib 2.0.11 added efficient indexing for
l1
andlinf
.So I added support for l1 spaceType and scoring.
I did not add new scoring to knn whitelist as it needs review.