opendistro-for-elasticsearch / k-NN

🆕 A machine learning plugin which supports an approximate k-NN search algorithm for Open Distro.
https://opendistro.github.io/
Apache License 2.0
277 stars 55 forks source link

Add support for L1 distance in AKNN, custom scoring and painless scripting #310

Closed elb3k closed 3 years ago

elb3k commented 3 years ago

Feature request from this issue

nmslib 2.0.11 added efficient indexing for l1 and linf. So I added support for l1 spaceType and scoring. I did not add new scoring to knn whitelist as it needs review.

codecov[bot] commented 3 years ago

Codecov Report

Merging #310 (22d4662) into master (b34f930) will increase coverage by 0.10%. The diff coverage is 89.47%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ Complexity Δ
...oforelasticsearch/knn/index/util/KNNConstants.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...asticsearch/knn/plugin/script/KNNScoringSpace.java 94.73% <75.00%> (-5.27%) 0.00 <0.00> (ø)
...endistroforelasticsearch/knn/index/SpaceTypes.java 100.00% <100.00%> (ø) 5.00 <0.00> (ø)
...arch/knn/plugin/script/KNNScoringSpaceFactory.java 90.90% <100.00%> (+2.02%) 5.00 <0.00> (+1.00)
...lasticsearch/knn/plugin/script/KNNScoringUtil.java 98.27% <100.00%> (+0.23%) 21.00 <3.00> (+3.00)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b34f930...22d4662. Read the comment docs.

jmazanec15 commented 3 years ago

Hi @Elbek-Khoshimjonov, thanks for the PR! I will review this week,

elb3k commented 3 years ago

Hi @Elbek-Khoshimjonov, overall this change looks great! A couple minor comments:

  1. 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
  2. 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
  3. 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!

VijayanB commented 3 years ago

can you add the corresponding method to below file like l2Squared? https://github.com/opendistro-for-elasticsearch/k-NN/blob/master/src/main/resources/com/amazon/opendistroforelasticsearch/knn/plugin/script/knn_whitelist.txt

Can you also corresponding tests here? https://github.com/opendistro-for-elasticsearch/k-NN/blob/master/src/test/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/PainlessScriptScoringIT.java

elb3k commented 3 years ago

can you add the corresponding method to below file like l2Squared? https://github.com/opendistro-for-elasticsearch/k-NN/blob/master/src/main/resources/com/amazon/opendistroforelasticsearch/knn/plugin/script/knn_whitelist.txt

Can you also corresponding tests here? https://github.com/opendistro-for-elasticsearch/k-NN/blob/master/src/test/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/PainlessScriptScoringIT.java

Done.

VijayanB commented 3 years ago

LGTM! Thanks for making changes!