-
Notifications
You must be signed in to change notification settings - Fork 72
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
Integrate explainability for hybrid query into RRF processor #1037
Integrate explainability for hybrid query into RRF processor #1037
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/rrf-score-normalization-v2 #1037 +/- ##
========================================================================
+ Coverage 78.49% 78.53% +0.03%
- Complexity 1070 1075 +5
========================================================================
Files 90 91 +1
Lines 3729 3745 +16
Branches 619 619
========================================================================
+ Hits 2927 2941 +14
- Misses 541 543 +2
Partials 261 261 ☔ View full report in Codecov by Sentry. |
8632f9c
to
3d67d74
Compare
Signed-off-by: Martin Gaievski <[email protected]>
3d67d74
to
2c5e6d0
Compare
Signed-off-by: Martin Gaievski <[email protected]>
06532d1
to
86c3263
Compare
@@ -111,8 +111,9 @@ public SearchResponse processResponse( | |||
); | |||
} | |||
// Create and set final explanation combining all components | |||
Float finalScore = Float.isNaN(searchHit.getScore()) ? 0.0f : searchHit.getScore(); |
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.
When sorting by a field other than score
:
searchHit.score
will beFloat.NaN
score
in the search hit and in actual response will benull
We can't pass null
as a value for the explanation object. Therefore, I've set it to 0.0
in these cases. This ensures that we always have a valid numeric value for the explanation, even when the score isn't the primary sorting factor.
@ToString.Include | ||
public static final String TECHNIQUE_NAME = "rrf"; | ||
|
||
// Not currently using weights for RRF, no need to modify or verify these params | ||
public RRFScoreCombinationTechnique(final Map<String, Object> params, final ScoreCombinationUtil combinationUtil) {} |
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.
those parameters were never used
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!
Should we add more unit test to increase test coverage? |
Actual coverage should be higher than that. I've added several unit tests, and the run that is currently in this PR, is old (12/18 ~2pm PST). All next runs got throttled because of the shared limited quota for uploads (example) |
1d93192
into
opensearch-project:feature/rrf-score-normalization-v2
Description
Adding explainability support to RRF processor
This PR adds explainability functionality to the RRF (Reciprocal Rank Fusion) processor. It's being submitted to the feature branch while the application security review for the RRF feature is in progress.
Key changes:
score
, we return0.0
as the score in explanationPR for explainability feature #1014
Example response when RRF is part of the search pipeline and the 'explain' flag is set:
Check List
[ ] New functionality has been documented.[ ] API changes companion pull request created.--signoff
.[ ] Public documentation issue/PR created.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.