-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove epoch from trainer.logged_metrics
#9904
Conversation
logged_metrics
trainer.logged_metrics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT !
Codecov Report
@@ Coverage Diff @@
## master #9904 +/- ##
======================================
Coverage 93% 93%
======================================
Files 178 178
Lines 15647 15652 +5
======================================
+ Hits 14502 14508 +6
+ Misses 1145 1144 -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think that the automatic logging of the epoch is very useful and that it should stay.
It is extremely useful for converting steps to epochs and vice versa in metric plots as it is monotonic.
I propose to fix this by logging the epoch inside the loop manually at the right place instead of doing it in the connector here. What do you think?
@awaelchli epoch will be logged. nothing is changed on the logger side. it's just that we won't be storing that in |
What does this PR do?
Fixes #9825
Reason why I think epoch is not required here:
logged_metrics
since it's not something that is aggregated or computed.*_step
hooks then it won't be correct. We already havetrainer.current_epoch
for that.self.log('step', some_step)
then we don't keepstep
inside logged_metrics.But as always open to discussion/suggestions.
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃