-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Add Bounded Window to Inference Models for Rescoring to Ensure Positive Score Range #125694
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 Bounded Window to Inference Models for Rescoring to Ensure Positive Score Range #125694
Conversation
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
this(model, model.getMinPredictedValue(), model.getMaxPredictedValue()); | ||
} | ||
|
||
public BoundedWindowInferenceModel(BoundedInferenceModel model, double minPredictedValue, double maxPredictedValue) { |
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.
The absolute minimum of 0
is only for LTR models but we can imagine situations were we want to scale the result of a regression model in [-1; 1].
I would personally remove the adjusment from the constructor cause it does not make sense:
public BoundedWindowInferenceModel(BoundedInferenceModel model, double minPredictedValue, double maxPredictedValue) {
this.model = model;
this.minPredictedValue = minPredictedValue;
this.maxPredictedValue = maxPredictedValue;
}
Then you can create a static method (scaleLtrModel
?) that would do the following:
public scaleLtrModel(BoundedInferenceModel model) {
int adjustment = LTR_MIN_PREDICTED_VALUE - model.getMinPredictedValue();
return new BoundedWindowInferenceModel(model, model.getMinPredictedValue() + adjustment, model.getMaxPredictedValue() + adjustment)
}
Then you can use the formula of the POC to scale the prediction:
double predictedValue = ((Number) regressionInferenceResults.predictedValue()).doubleValue();
// First we scale the data to [0 ,1]
predictedValue = (predictedValue - model.getMinPredictedValue()) / (model.getMaxPredictedValue() - model.getMinPredictedValue());
// Then we scale the data to the desired interval
predictedValue = predictedValue * (getMaxPredictedValue() - getMinPredictedValue()) + getMinPredictedValue();
Also I would rename the class into something like MinMaxScaledInferenceModel
WDYT?
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.
The absolute minimum of 0 is only for LTR models but we can imagine situations were we want to scale the result of a regression model in [-1; 1].
I thought we had decided with @jimczi that we would not perform scaling, but rather slide the scores up to ensure they are all positive (only if the minimum score was negative). Correct?
The absolute minimum of 0 is only for LTR models but we can imagine situations were we want to scale the result of a regression model in [-1; 1].
That makes sense. Is there a need for this now to fix this bug though? I was under the impression that the bug was only about the negative scores being returned and us having to deal with that if true.
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 proposed "sliding" the score because we can apply it on the minimum and maximum value for a model entirely (instead of per query). This means that scores will still be comparable between queries.
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.
Thanks @jimczi - what are you thoughts on the PR as-is then?
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.
Sounds good. Let's use the new minPredictedValue
and maxPredictedValue
from the BoundedInferenceModel
directly, no need to make it configurable for now.
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.
Then you can use the formula of the POC to scale the prediction:
double predictedValue = ((Number) regressionInferenceResults.predictedValue()).doubleValue(); // First we scale the data to [0 ,1] predictedValue = (predictedValue - model.getMinPredictedValue()) / (model.getMaxPredictedValue() - model.getMinPredictedValue());
Looking at this here -- would scaling in this method (normalizing it to 0 -> 1.0 first) fall victim to what we're trying to avoid here - that is, doing this may compress the space and lose precision for the scores and might cause closely scored items to have equal rank?
I think it's a good idea overall, and certainly more flexible for the developer though... so, I'm on the fence about it. cc: @jimczi
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.
@afoucret - the more I think about this, for the time being, let's go with this solution, and if we decide to add in your suggestion, let's do that afterwords so we can unblock your work.
Pinging @elastic/ml-core (Team:ML) |
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.
The approach looks good to me.
Pinging @elastic/ml-core for a validation here. We're enforcing all retrievers/query to return positive scores and this PR handles the tree inference models.
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…ve Score Range (elastic#125694) * apply bounded window inference model * linting * add unit tests * [CI] Auto commit changes from spotless * add additional tests * remove unused constructor --------- Co-authored-by: elasticsearchmachine <[email protected]> (cherry picked from commit e77bf80)
…ve Score Range (elastic#125694) * apply bounded window inference model * linting * add unit tests * [CI] Auto commit changes from spotless * add additional tests * remove unused constructor --------- Co-authored-by: elasticsearchmachine <[email protected]>
…ve Score Range (#125694) (#126149) * apply bounded window inference model * linting * add unit tests * [CI] Auto commit changes from spotless * add additional tests * remove unused constructor --------- Co-authored-by: elasticsearchmachine <[email protected]> (cherry picked from commit e77bf80)
…ve Score Range (elastic#125694) * apply bounded window inference model * linting * add unit tests * [CI] Auto commit changes from spotless * add additional tests * remove unused constructor --------- Co-authored-by: elasticsearchmachine <[email protected]> (cherry picked from commit e77bf80)
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…ve Score Range (#125694) (#127345) * apply bounded window inference model * linting * add unit tests * [CI] Auto commit changes from spotless * add additional tests * remove unused constructor --------- Co-authored-by: elasticsearchmachine <[email protected]> (cherry picked from commit e77bf80)
This PR allows LTR configuration model inference to get rid of negative score that can be issued by model trained using xgboost or other framework. The negative score can cause issues if used in Lucene where they are forbidden.
The implementation here checks the model's predictive bounds (when possible) to see if the minimum predicted value will be below zero, and if so adjust the scores upward so the minimum value is 0 by sliding all the predicted values upwards in the result (thus, keeping the rankings and the relativity of the scores to each other).