Skip to content
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

Fix LogEventStore end epoch log #1314

Merged
merged 1 commit into from Feb 19, 2024
Merged

Fix LogEventStore end epoch log #1314

merged 1 commit into from Feb 19, 2024

Conversation

laggui
Copy link
Member

@laggui laggui commented Feb 15, 2024

Checklist

  • Confirmed that run-checks all script has been executed.

Issue

Noticed that the reported validation epoch was off by 1 in the logs. For example, if you run the mnist example without early stopping to let it run for the full 10 epochs, you will see in the artifact dir:

$ ls train/
epoch-1  epoch-10  epoch-2  epoch-3  epoch-4  epoch-5  epoch-6  epoch-7  epoch-8  epoch-9

$ ls valid/
epoch-1  epoch-10  epoch-11  epoch-3  epoch-4  epoch-5  epoch-6  epoch-7  epoch-8  epoch-9

Notice how there is no epoch-2 in valid/ but the epochs go up to 11.

Changes

Removed the epoch + 1 in logger.end_epoch call for validation.

Not sure if this was incremented for historical reasons that are no longer relevant, but with this easy fix everything seems to be in order when I check the log folders.

Testing

Ran the mnist example again after and the epochs match for train and valid:

$ ls train/
epoch-1  epoch-10  epoch-2  epoch-3  epoch-4  epoch-5  epoch-6  epoch-7  epoch-8  epoch-9

$ ls valid/
epoch-1  epoch-10  epoch-2  epoch-3  epoch-4  epoch-5  epoch-6  epoch-7  epoch-8  epoch-9

@nathanielsimard nathanielsimard merged commit 80f3cdd into main Feb 19, 2024
13 checks passed
@nathanielsimard nathanielsimard deleted the fix/valid-event-log branch February 19, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants