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

Note about metrics in AbstractWrapperTeacher.act() #3160

Merged
merged 3 commits into from Oct 7, 2020

Conversation

EricMichaelSmith
Copy link
Contributor

Add to the docstring that self.task.act() must be called in the wrapper's .act() method for metrics to be recorded correctly

@github-actions
Copy link

github-actions bot commented Oct 6, 2020

Your PR contains a change to a task. Please paste the results of the following command into a comment:

python tests/datatests/test_new_tasks.py

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.

wait, most metrics are implemented in observe tho, I don't understand.

@EricMichaelSmith
Copy link
Contributor Author

wait, most metrics are implemented in observe tho, I don't understand.

Yes, but assuming that the wrapped teacher is a FixedDialogTeacher, if self.task.act() is not called, self.lastY will not get set and self.task.observe() will not be able to record metrics. But not calling self.task.act() would also lead to other problems like (most importantly) not getting the act Message from the wrapped teacher, so I've just generalized this language.

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.

maybe s/will/should

Thanks for the explanation

@EricMichaelSmith EricMichaelSmith merged commit 374ae30 into master Oct 7, 2020
@EricMichaelSmith EricMichaelSmith deleted the EricMichaelSmith-patch-1 branch October 7, 2020 13:19
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

3 participants