Explain API changes #2403
Explain API changes #2403
Conversation
neetikasinghal
force-pushed
the
explain
branch
from
93c1f96 to
8b52ed7
Compare
There was a problem hiding this comment.
We need to have Unit test for each case
- Disk-based search with valid rescore context.
- Radial search.
- ANN search (default case).
- Shard-level rescoring enabled.
- Shard-level rescoring disabled.
- Filter weight case where filtered IDs are less than k.
- Filter threshold value greater than cardinality.
- Missing native engine files.
- Valid context with matching document and disk-based search.
And Please validate with Explaination Object.
@Vikasht34 yes the tests are not yet added in here, hence its in a draft status. I will add the coverage with all the possible cases. |
neetikasinghal
force-pushed
the
explain
branch
2 times, most recently
from
bffbdac to
7c4f425
Compare
neetikasinghal
requested review from
heemin32,
navneet1v,
VijayanB,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski,
ryanbogan,
luyuncheng,
shatejas and
0ctopus13prime
as code owners
|
@navneet1v / @Vikasht34 would you please review the changes? |
|
@neetikasinghal can you please add an entry in the change log |
yup i generally add it towards the end of the review so that its easier to rebase with the latest changes. |
yup, that WIP as my setup for ITs was broken. I am able to fix that now, however the PR has the coverage for all the UTs. |
src/main/java/org/opensearch/knn/index/query/explain/KnnExplanation.java
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/query/common/DocAndScoreQuery.java
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/query/common/DocAndScoreQuery.java
Show resolved
Hide resolved
Signed-off-by: Neetika Singhal <[email protected]>
neetikasinghal
force-pushed
the
explain
branch
from
8745703 to
8b90830
Compare
|
pending items -
|
| public Explanation explain(LeafReaderContext context, int doc, float score, KNNScorer knnScorer) { | ||
| knnQuery.setExplain(true); | ||
| try { | ||
| knnScorer = getOrCreateKnnScorer(context, knnScorer); |
There was a problem hiding this comment.
is there a scenario when knnScorer is not null? If no, then you should not pass it to this method and always create new instance inside the method.
| try { | ||
| return FieldInfoExtractor.getSpaceType(modelDao, fieldInfo); | ||
| } catch (IllegalArgumentException e) { | ||
| return knnQuery.getVectorDataType() == VectorDataType.BINARY ? SpaceType.DEFAULT_BINARY : SpaceType.DEFAULT; |
There was a problem hiding this comment.
why we are not signaling to user that system is in bad state, wouldn't this be misleading, I mean giving false data to the user?
| if (explanationFormula != null) { | ||
| return explanationFormula; | ||
| } | ||
| throw new UnsupportedOperationException("explainScoreTranslation is not defined for this space type."); |
There was a problem hiding this comment.
this effectively means if object created with old constructor (formula is null) then this method will throw exception, is this desired behavior? Can we have some constant like "undefined" or similar, or if formula isn't defined the rets of explanation detail don't make sense as well?
| public class KnnExplanation { | ||
|
|
||
| @Getter | ||
| private final Map<Object, Integer> annResultPerLeaf; |
There was a problem hiding this comment.
are you sure you want to expose raw map? clients will be able to modify them. You may want to consider returning Collections.unmodifiableMap(your_map) in getter method
Description
Add support for explain for Exact/ANN/Radial/Disk/Filtering k-nn search. Score calculation explanation is currently added only for ANN search.
Proposal for explain is given here: #875 (comment)
ITs - WIP..
Related Issues
Resolves #875
Check List
--signoff.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.