Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Added DST Teacher for multiwoz_v22 task #4656

Merged
merged 12 commits into from Jul 28, 2022
Merged

Conversation

prajjwal1
Copy link
Contributor

@prajjwal1 prajjwal1 commented Jul 7, 2022

The DST Teacher for multiwoz_v22 is not present. Adding this teacher would allow multiwoz_v22 to be usable for Dialog State Tracking task.

cc @chinnadhurai

@@ -373,6 +379,212 @@ def get_id_task_prefix(self):
return "MultiwozV22"


class MultiWOZv22DSTTeacher(tod_agents.TodUserSimulatorTeacher):
Copy link
Contributor

Choose a reason for hiding this comment

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

might be better to add a comment saying this teacher is needed for reproducing the Joint Goal Accuracy values reported in the simpleTOD & SOLOIST papers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better now ? @chinnadhurai

@moyapchen
Copy link
Contributor

Looks okay code-wise, though I haven't done a through review. Should this be in ParlAI-Internal rather than public ParlAI though?

@@ -393,5 +605,5 @@ class SingleApiSchemaAgent(MultiwozV22Parser, tod_agents.TodSingleApiSchemaAgent
pass


class DefaultTeacher(SystemTeacher):
Copy link
Contributor

Choose a reason for hiding this comment

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

would be better to keep the default to SystemTeacher as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

@chinnadhurai
Copy link
Contributor

Looks okay code-wise, though I haven't done a through review. Should this be in ParlAI-Internal rather than public ParlAI though?

we need this teacher for the actual DST task in Multiwoz and reproduce JGA reported in the SOTA models. So, figured it might make sense to push to the public one.

@prajjwal1
Copy link
Contributor Author

@stephenroller Can you please look into this ? It has been here for sometime now.

Copy link
Contributor

@stephenroller stephenroller left a comment

Choose a reason for hiding this comment

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

I'll defer to @moyapchen ans @klshuster here. Don't see any issue though

Copy link
Contributor

@klshuster klshuster left a comment

Choose a reason for hiding this comment

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

deferring to @moyapchen for final say but looks OK to me

could we generate test files for it as well?

@prajjwal1
Copy link
Contributor Author

@moyapchen Can you please see this ?

Copy link
Contributor

@moyapchen moyapchen left a comment

Choose a reason for hiding this comment

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

lgtm

@prajjwal1 prajjwal1 merged commit 8707b32 into facebookresearch:main Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants