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

Add Metric.from_mask helper method (#3411) #4894

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

poojasethi
Copy link
Contributor

@poojasethi poojasethi commented Nov 23, 2022

Patch description

This change introduces a new from_mask helper function in the Metric class. It also refactors the compute_loss function in torch_generator_agent.py to call the from_mask helper when computing the loss, ppl, and token_acc.

Testing steps

Unit tests

pytest -v tests/test_metrics.py
Note that I added two new unit tests, test_average_metric_from_mask and test_ppl_metric_from_mask.

Manual logging

I manually verified loss, ppl, token_acc, and token_em in torch_generator_agent.py by logging both the original and new values like this and running the below command:

 parlai train_model --model examples/seq2seq \           
    --model-file /tmp/example_model \
    --task convai2 --batchsize 32 --num-epochs 2 --truncate 128 --seed 42 > metric_from_mask.txt

We can see that old_loss equals loss, old_ppl equals ppl, etc.

2022-11-22 17:35:17,429 INFO     | time:192s total_exs:6400 total_steps:200 epochs:0.05 time_left:7687s
    clen  clip  ctpb  ctps  ctrunc  ctrunclen  exps  exs  gnorm  llen  loss  lr  ltpb  \
   140.9     1  3457  3633   .5481      32.84 33.63 1600  15.06 12.85 8.829   1 411.2   
    ltps  ltrunc  ltrunclen  old_loss  old_ppl  old_token_acc  old_token_em  ppl  \
   432.1       0          0     8.829     6827          .1721             0 6827   
    token_acc  token_em  total_train_updates  tpb  tps   ups  
        .1721         0                  200 3868 4065 1.051

Other information

  • I believe some of these files could potentially be refactored to use the new helper function as well. Happy to submit another PR, if helpful!
  • I tried running autoformat.sh but it was running quite slowly... Tips on how to use this script would be helpful :)

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 looks like it fixes that old issue! Actually @mojtaba-komeili looks like he may be able to use this.

Leaving one comment to see if we can go even one step cleaner

"""
tokens_per_ex = mask.long().sum(dim=-1)
metric_per_ex = (metric_per_token * mask).sum(dim=-1)
metrics = MyMetric.many(metric_per_ex, tokens_per_ex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use cls.many instead and get away without passing MyMetric as an extra parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, yes! Good idea.

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.

lgtm!

@poojasethi
Copy link
Contributor Author

Sweet! @stephenroller and @klshuster should I wait for / help dig into the lint and CircleCI failures, or go ahead and land the PR?

@poojasethi poojasethi merged commit 05ff609 into facebookresearch:main Nov 29, 2022
@poojasethi
Copy link
Contributor Author

Landed! (Discussed with @klshuster offline and it seems that the lint and CircleCI failures are not related to this PR)

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.

3 participants