Create explainable user model classes #223

IKostric · 2023-09-26T09:15:54Z

No description provided.

IKostric · 2023-09-26T09:16:24Z

Simple Explainable user model based on tags.

github-actions · 2023-09-26T09:21:08Z

Current Branch	Main Branch

NoB0

Mostly nits and suggestions. As for PR #184, we may need few iterations.

NoB0 · 2023-10-02T11:20:42Z

moviebot/explainability/explainable_user_model.py

+    def generate_explanation(
+        self, user_preferences: UserPreferences
+    ) -> AnnotatedUtterance:
+        """Generate an explanation based on the provided input data.


Suggested change

"""Generate an explanation based on the provided input data.

"""Generates an explanation based on the provided input data.

NoB0 · 2023-10-02T11:21:31Z

moviebot/explainability/explainable_user_model.py

+            generated.
+
+        Returns:
+            The generated explanation.


Suggestion: Maybe this could be more precise, like utterance containing explanation.

+1 I was wondering why the return type was AnnotatedUtterance, so it would be worth mentioning here that the explanation is to be returned as a system utterance.

NoB0 · 2023-10-02T11:22:28Z

moviebot/explainability/explainable_user_model_tag_based.py

+    UserPreferences,
+)
+
+_DEFAULT_TEMPLATE_FILE = "moviebot/explainability/explanation_templates.yaml"


I would argue to put this file in the folder data.

NoB0 · 2023-10-02T11:22:47Z

moviebot/explainability/explainable_user_model_tag_based.py

+
+class ExplainableUserModelTagBased(ExplainableUserModel):
+    def __init__(self, template_file: str = _DEFAULT_TEMPLATE_FILE):
+        """Initialize the ExplainableUserModelTagBased class.


Nit: conjugate verb in docstring.

Suggested change

"""Initialize the ExplainableUserModelTagBased class.

"""Initializes the ExplainableUserModelTagBased class.

NoB0 · 2023-10-02T11:23:57Z

moviebot/explainability/explainable_user_model_tag_based.py

+        Args:
+            template_file: Path to the YAML file containing explanation
+            templates. Defaults to _DEFAULT_TEMPLATE_FILE.
+        """


Suggestion: check if file exists raise exception otherwise.

NoB0 · 2023-10-02T11:24:14Z

moviebot/explainability/explainable_user_model_tag_based.py

+    def generate_explanation(
+        self, user_preferences: UserPreferences
+    ) -> AnnotatedUtterance:
+        """Generate an explanation based on the provided user preferences.


Nit: conjugate verb in docstring.

Suggested change

"""Generate an explanation based on the provided user preferences.

"""Generates an explanation based on the provided user preferences.

NoB0 · 2023-10-02T11:25:46Z

moviebot/explainability/explainable_user_model_tag_based.py

+        """Generate an explanation based on the provided user preferences.
+
+        Args:
+            user_preferences: Nested dictionary of user preferences.


Nit: simply user preferences?

NoB0 · 2023-10-02T11:27:25Z

moviebot/explainability/explainable_user_model_tag_based.py

+
+        Args:
+            template: Template containing negative keyword.
+            remove: If True, remove the negative keyword.


Nit: add defaults value.

Suggested change

remove: If True, remove the negative keyword.

remove: If True, remove the negative keyword. Defaults to True.

NoB0 · 2023-10-02T11:28:27Z

tests/explainability/test_explainable_user_model_tag_based.py

+
+
+@pytest.fixture
+def explainable_model():


Nit: missing return type.

NoB0 · 2023-10-02T11:31:51Z

tests/explainability/test_explainable_user_model_tag_based.py

+@pytest.fixture
+def explainable_model():
+    return ExplainableUserModelTagBased(
+        "moviebot/explainability/explanation_templates.yaml"


I am wondering if it you be best to make _DEFAULT_TEMPLATE_FILE public and reuse it here?

We dont even need to make it public. It defaults to the default path :)

kbalog

Assuming that UserPreferences contains item-based preferences, there is a missing step of inferring preferences for tags. That should take place in this class (by talking to the movie DB).

kbalog · 2023-10-04T16:44:41Z

moviebot/explainability/explainable_user_model.py

+            generated.
+
+        Returns:
+            The generated explanation.


+1 I was wondering why the return type was AnnotatedUtterance, so it would be worth mentioning here that the explanation is to be returned as a system utterance.

kbalog · 2023-10-04T17:47:43Z

moviebot/explainability/explainable_user_model_tag_based.py

@@ -0,0 +1,81 @@
+"""This module contains the ExplainableUserModelTagBased class.


Most modules contain a single class, so this docstring is not that informative. Suggestion instead: "Tag-based user model explanation."

kbalog · 2023-10-04T17:48:48Z

moviebot/explainability/explainable_user_model_tag_based.py

+
+        Args:
+            template_file: Path to the YAML file containing explanation
+            templates. Defaults to _DEFAULT_TEMPLATE_FILE.


Nit: second line should be indented.

kbalog

accidentally approved it, ignore

kbalog

see comments

IKostric · 2023-10-17T23:37:27Z

Assuming that UserPreferences contains item-based preferences, there is a missing step of inferring preferences for tags. That should take place in this class (by talking to the movie DB).

Since we have attribute preferances readily available, we can start with those. Later, we can expand to item-based preferances.

kbalog

LGTM (see one nit)

kbalog · 2023-10-27T08:28:50Z

moviebot/explainability/explainable_user_model_tag_based.py

-The class generates explanations for user preferences in the movie domain based
-on templates loaded from a YAML file.
+The class generates explanations for user preferences in the movie domain.
+Currently, the explanations are based on the explicit movie tags/attributes.


Nit: "... are based on movie tags/attributes that were explicitly mentioned by the user in the conversation."

Create explainable user model classes

2150bbb

IKostric requested review from kbalog and NoB0 September 26, 2023 09:16

NoB0 approved these changes Oct 2, 2023

View reviewed changes

kbalog requested changes Oct 4, 2023

View reviewed changes

kbalog approved these changes Oct 6, 2023

View reviewed changes

kbalog requested changes Oct 6, 2023

View reviewed changes

IKostric requested a review from kbalog October 17, 2023 23:35

IKostric added 2 commits October 18, 2023 01:38

Address review comments

af729d7

Update docstrings

2600bd3

kbalog approved these changes Oct 27, 2023

View reviewed changes

Expand docstrings

8c68b1d

IKostric merged commit cc39825 into main Oct 27, 2023
2 checks passed

IKostric deleted the feature/222-Add-ExplainableUserModel-Class branch October 27, 2023 08:52

IKostric mentioned this pull request Feb 26, 2024

Add user model to DST #248

Closed

	"""Generate an explanation based on the provided input data.
	"""Generates an explanation based on the provided input data.

	"""Initialize the ExplainableUserModelTagBased class.
	"""Initializes the ExplainableUserModelTagBased class.

	remove: If True, remove the negative keyword.
	remove: If True, remove the negative keyword. Defaults to True.

		@@ -0,0 +1,81 @@
		"""This module contains the ExplainableUserModelTagBased class.

Create explainable user model classes #223

Create explainable user model classes #223

Conversation

IKostric commented Sep 26, 2023

IKostric commented Sep 26, 2023

github-actions bot commented Sep 26, 2023

NoB0 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IKostric Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

kbalog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbalog left a comment • edited Loading

Choose a reason for hiding this comment

kbalog left a comment

Choose a reason for hiding this comment

IKostric commented Oct 17, 2023

kbalog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoB0 left a comment •

edited

Loading

IKostric Oct 17, 2023 •

edited

Loading

kbalog left a comment •

edited

Loading