Skip to content
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

Contextual Information with Neural NLU #258

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joyce922
Copy link
Collaborator

I made the necessary changes for catching the contextual information from the given text. Also after running the moviebot to load the necessary slot values I am also adding extra slot values for contextual information.

I made the necessary changes for catching the contextual information from the given text. Also after running the moviebot to load the necessary slot values I am also adding extra slot values for contextual information.
@IKostric IKostric self-requested a review April 30, 2024 20:47
Copy link
Collaborator

@IKostric IKostric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we need to differentiate between "context" and "slot" when annotating. They will be modeled similarly anyway.

When writing unit tests, we assume we have the "perfect model". This means we can mock it (_model.predict method) and set the outputs exacly to what we expect. Then we only write tests for different branches in the method we are testing.

"title",
"time",
"companion",
"location"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe slots_annotation is used to fetch information from the database so it might break in the current version. Do we need to extend this list in this PR?

- "[I always look forward](modifier) to watching [Denzel Washington](actors) movies on [Friday nights](time) with my [parents](companion)."
- "[I'm hooked on](modifier) watching movies with my [roommate](companion) on [weekend nights](time) on [HBO](location)."
- "[I enjoy](modifier) watching [Tom Cruise](actors) movies on [Sunday afternoons](time) on [television](location)."
- "[I savor](modifier) watching [Sacha Baron Cohen](actors) movies on [Thursday evenings](time) with my [friends](companion) on my [laptop](location)."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this enough samples, or do we need to extend?

B_PREFERENCE_COMPANION = auto()
I_PREFERENCE_COMPANION = auto()
B_PREFERENCE_LOCATION = auto()
I_PREFERENCE_LOCATION = auto()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add CONTEXT in the name so we know what these refer to. E.g., B_PREFERENCE_CONTEXT_TIME

"start": char_start,
"end": char_end,
}
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need to differentiate between "context" and "slot"? We want to use them in a similar way, so the extraction can be the same. "context" should be un the slot name if you address my comment in slot_mapping.py.

@@ -173,3 +189,4 @@ class DS:
)

print([str(da) for da in da])
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire block (if __name__ == "__main__") should be removed. Use unit testing instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants