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

ModelCheckpoint period should not always save on the first epoch #3619

Closed
carmocca opened this issue Sep 23, 2020 · 3 comments · Fixed by #3630
Closed

ModelCheckpoint period should not always save on the first epoch #3619

carmocca opened this issue Sep 23, 2020 · 3 comments · Fixed by #3630
Assignees
Labels
bug Something isn't working feature Is an improvement or enhancement help wanted Open to be worked on
Milestone

Comments

@carmocca
Copy link
Contributor

🚀 Feature

period should work so:

# ModelCheckpoint on_validation_end
if (epoch + 1) % period:
    # do not save
    return

e.g:

  • period == 1: save on epoch 0, 1, 2...
  • period == 2: save on epoch 1, 3, 5...
  • period == 3: save on epoch 2, 5, 8...

currently, it always runs on the first epoch and then runs every period epochs

e.g:

  • period == 1: save on epoch 0, 1, 2...
  • period == 2: save on epoch 0, 2, 4...
  • period == 3: save on epoch 0, 3, 6...

This would also allow having period = 0 which would never save, just as save_top_k = 0

Motivation

I want to save a checkpoint every period epochs but current behaviour forces to always save on the first one.

@carmocca carmocca added feature Is an improvement or enhancement help wanted Open to be worked on labels Sep 23, 2020
@williamFalcon
Copy link
Contributor

makes sense. submit a PR?

@williamFalcon williamFalcon added the bug Something isn't working label Sep 23, 2020
@williamFalcon williamFalcon added this to the 1.0 milestone Sep 23, 2020
@ananthsub
Copy link
Contributor

@williamFalcon why is epoch 0-indexed in one place and 1-indexed in other places? can it be consistent across lightning instead of these +1s? I think the same issue affects batch indices too

@williamFalcon
Copy link
Contributor

yes, we did a refactor a while to make everything zero indexed. where did we miss spots?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature Is an improvement or enhancement help wanted Open to be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants