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

Expose Precision and Recall to user via TeacherMetrics #4670

Merged
merged 12 commits into from
Aug 9, 2022

Conversation

prajjwal1
Copy link
Contributor

Patch description
Considering that other metrics such as ctpb, ctrunc, ctrunclen (less frequently used by user) are exposed, it makes a lot of sense to expose precision and recall as they are internally being computed.

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.

This is cool but I think it will be slow from recomputing these values?

@prajjwal1
Copy link
Contributor Author

The way this function is setup, extracting f, p and r all at once will break other functionalities. I've been using it for sometime now, doesn't seem to add much overhead. Most of the time bs is set to <256, so should be fine. I can make it return all three things with one run, but then some other things will have to be changed. Do you have suggestions ?

@prajjwal1
Copy link
Contributor Author

Or I can make "f1" default. If the user wants, they can print precision and recall themselves by passing the mode to output parameter.

@prajjwal1
Copy link
Contributor Author

@stephenroller Can you please provide what you think based on the reply ?

@stephenroller
Copy link
Contributor

Sorry; compute and return all 3 metrics in a generalization of the existing utility method. The caller will be responsible for separating and accumulating them.

@prajjwal1
Copy link
Contributor Author

@stephenroller Is it fine now ? We only compute p, r and f1 only once now. So runtime is not affected.

@prajjwal1 prajjwal1 requested a review from klshuster July 29, 2022 21:56
@prajjwal1
Copy link
Contributor Author

@klshuster Can you please take a look ?

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.

this looks great. could you please update the metrics master list (https://github.com/facebookresearch/ParlAI/blob/main/parlai/core/metrics.py#L53) to have the new metrics, and also please rename 'precision' and 'recall' to refer to the fact that they are computed based on unigrams (e.g., uni_prec)

@prajjwal1 prajjwal1 requested a review from klshuster August 4, 2022 17:43
@prajjwal1 prajjwal1 merged commit 1186e81 into facebookresearch:main Aug 9, 2022
@klshuster klshuster mentioned this pull request Aug 17, 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.

4 participants