-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Policy Predictions for End-to-End #7136
Conversation
955ad22
to
36e73a7
Compare
from unittest.mock import Mock, patch | ||
|
||
import numpy as np | ||
import pytest | ||
from _pytest.monkeypatch import MonkeyPatch |
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.
sorry, typing got a bit out of hand here 🙈
36e73a7
to
4f5c697
Compare
db327b1
to
47dacd6
Compare
I'll have an engineer review this as soon as you're ok with the changes in general @Ghostvv |
part of #6670 |
20e64bd
to
da6c99d
Compare
@degiz Do you have time? It looks bigger than it is. Mostly typing and renames. |
@wochinge can review that tomorrow in the morning, would it be fine? |
Sure, absolutely no rush! |
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.
Great job adding all the docstrings, comments, types and tests 🚀 🚀
@staticmethod | ||
def is_not_memo_policy( | ||
policy_name: Text, max_confidence: Optional[float] = None | ||
policy_name: Optional[Text], max_confidence: Optional[float] = None |
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.
Should the default value of max_confidence
be 0.0
?
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.
No as this would otherwise evaluate to True
for the call here
rasa/core/policies/ensemble.py
Outdated
) -> bool: | ||
"""Checks if the policy is the `MemoizationPolicy` or one of its subclasses. |
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.
That's not exactly what it's doing right? There's also a check of max_confidence
argument.
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.
re-named the function and changed the docstring. Better?
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.
🚀
Co-authored-by: Alexander Khizov <[email protected]>
Proposed changes:
Policy
s now return aPolicyPrediction
object instead of the plain probabilities. This allows us to return multiple pieces after a prediction. This is not only useful for this PR but also for this PRPolicyEnsemble
now returns aPolicyPrediction
object instead ofTuple[probabilities, policy_name]
. The reason is the same for the change in thePolicy
interface: This allows us to encapsulate more information and pass this up to theMessageProcessor
test_policies
modulepredict_next_with_tracker
to avoid having to reimplement some predict logic withinserver
followup_action
Status (please check what you already did):
black
(please check Readme for instructions)