UnexpecTEDIntentPolicy predictions fix
#9079
Conversation
dakshvar22
requested review from
joejuzl and
wochinge
and removed request for
joejuzl
| @@ -235,6 +236,11 @@ def is_action_unlikely_intent(event: Event) -> bool: | |||
| return ( | |||
| type(event) == ActionExecuted | |||
| and event.action_name == ACTION_UNLIKELY_INTENT_NAME | |||
| ) or ( | |||
| type(event) == WronglyPredictedAction | |||
There was a problem hiding this comment.
Do we still need this now we have WarningPredictedAction?
There was a problem hiding this comment.
Also do we need any check for these test events here? They are not actually predicted ever right?
rasa/core/test.py
Outdated
| if has_prediction_target_mismatch or ( | ||
| predicted_warning and predicted_action != expected_action | ||
| if action_executed_eval_store.has_prediction_target_mismatch() or ( | ||
| predicted_action_unlikely_intent and predicted_action != expected_action |
There was a problem hiding this comment.
Isn't this part now the equivalent of previously_predicted_action_unlikely_intent?
There was a problem hiding this comment.
No, because we make another prediction => predicted_action gets overwritten
| if has_prediction_target_mismatch or ( | ||
| predicted_warning and predicted_action != expected_action | ||
| if action_executed_eval_store.has_prediction_target_mismatch() or ( | ||
| predicted_action_unlikely_intent and predicted_action != expected_action | ||
| ): | ||
| partial_tracker.update( | ||
| WronglyPredictedAction( |
There was a problem hiding this comment.
If we have an predict action_unlikely_intent but it's not in the story, and the following action is correct, won't be now end up with two WronglyPredictedAction in the tracker?
There was a problem hiding this comment.
If we predict action_unlikely_intent then we ask for another prediction, and only add this WronglyPredictedAction if there is a mismatch in expected/predicted. This is the only case when two WronglyPredictedActions will be added but that's perfectly fine because we filter out action_unlikely_intents from logs.
There was a problem hiding this comment.
Ok but if predicted_action_unlikely_intent and predicted_action != expected_action on line 584 is False, then predicted_action is not changed because we don't re-run so we no predicted_action_unlikely_intent and predicted_action != expected_action on line 609 will also always be False. And if line 584 is True then we know the next action can't be another action_unlikely_intents right? So can we change 609 to just if action_executed_eval_store.has_prediction_target_mismatch()?
There was a problem hiding this comment.
That's correct. Changed that.
rasa/core/test.py
Outdated
| if ( | ||
| fail_on_prediction_errors | ||
| and predicted_action != ACTION_UNLIKELY_INTENT_NAME | ||
| and expected_action != ACTION_UNLIKELY_INTENT_NAME |
There was a problem hiding this comment.
Doesn't this break the requirement we fail if the test story contains ACTION_UNLIKELY_INTENT_NAME but we don't predict it?
There was a problem hiding this comment.
Updated the condition and added a test for it (test_action_unlikely_intent_fail_on_prediction_errors)
rasa/core/test.py
Outdated
| @@ -667,6 +638,16 @@ def _collect_action_executed_predictions( | |||
| "training stories and retrain." | |||
| ) | |||
| raise WrongPredictionException(error_msg) | |||
| elif previously_predicted_action_unlikely_intent: | |||
There was a problem hiding this comment.
is this possible given that we already have predicted_action_unlikely_intent and predicted_action != expected_action in the or of the associated if?
There was a problem hiding this comment.
It is possible. prev_action_unlikely_intent is different from predicted_action_unlikely_intent and predicted_action != expected_action (see comment above)
alwx
requested review from
wochinge and
dakshvar22
and removed request for
wochinge
…into intent-ted-wrong-prediction
Proposed changes:
Status (please check what you already did):
black(please check Readme for instructions)