Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Log batch metrics #5362

Merged
merged 9 commits into from
Aug 19, 2021
Merged

Log batch metrics #5362

merged 9 commits into from
Aug 19, 2021

Conversation

dirkgr
Copy link
Member

@dirkgr dirkgr commented Aug 17, 2021

Fixes #5351.

@dirkgr dirkgr requested a review from epwalsh August 17, 2021 20:53
@dirkgr dirkgr self-assigned this Aug 17, 2021
@dirkgr dirkgr mentioned this pull request Aug 17, 2021
10 tasks
allennlp/training/callbacks/log_writer.py Outdated Show resolved Hide resolved
Comment on lines 245 to 246
if key.startswith("batch_"):
continue
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we don't log any metrics in metrics that start with batch_? Is that really what we want?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to avoid creating "batch_batch_loss" and "batch_batch_reg_loss", and I generalized a little. It doesn't make a ton of sense to define a metric called batch_*, because all metrics that come from the model are also calculated per epoch.

Copy link
Member

Choose a reason for hiding this comment

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

batch_size is one exception I can think of

Copy link
Member

Choose a reason for hiding this comment

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

Since we already handle batch_loss and batch_reg_loss separately, how we just explicitly ignore those here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll do that as soon ad I can get GitHub to talk to me again :-/

Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

🙂

@dirkgr dirkgr enabled auto-merge (squash) August 19, 2021 18:10
@dirkgr dirkgr merged commit 13de38d into main Aug 19, 2021
@dirkgr dirkgr deleted the LogMetrics branch August 19, 2021 18:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LogWriter log_batch missing metrics?
2 participants