save_best,-save_all,-save_last ++ bug fix

[ceyzaguirre4]

Before submitting

• Was this discussed/approved via a Github issue? Yes, Checkpoint should support two types of checkpoints, last and best #285
• Did you read the contributor guideline? YES
• Did you make sure to update the docs? YES

What does this PR do?

Implements #285.

[Borda]

the bool paradigm makes the parameter list too long and also the name convention of saved models needs discussion

[Borda]

why is the prefix used twice - here and also in the file name?

[ceyzaguirre4]

ModelCheckpoint.save_model had a bug (#394) where the whole directory is wiped. Passing self.prefix to the method allows me to then identify the files that need deletion by using re (re.match(r'{}_ckpt_epoch_\d+.ckpt'.format(prefix), filename))

[Borda]

see #128

[ceyzaguirre4]

Ok, yes I saw it
Three options:

• we merge this PR and when change Checkpoint callback's save_best_only to save_top_k #128 is ready they merge too.
• I wait until change Checkpoint callback's save_best_only to save_top_k #128 is ready (next month hopefully), rebase and add my changes to that.
  (- I rebase to current state of change Checkpoint callback's save_best_only to save_top_k #128 and try to fix the bugs there)

[Borda]

@williamFalcon which option do you prefer?
it is not about who is faster, but which philosophy to adopt

• [yours] separate saving best/all/last with name differentiation
• [other] saving N last/best models which look more general

[williamFalcon]

@ceyzaguirre4 this is awesome. I do agree that the supported cases should be:

1. n last
2. n best

Do we really need all? wouldn't people just set n=huge_number?

[Borda]

I would prefer N best...

[williamFalcon]

n last may not be the n best

[Borda]

I know, but what would it be the case you want to keep n last even they are quite bad?
I understand to keel all metrics abut not bad models... :)

[williamFalcon]

this is more of a research question. i can’t predict how people will use this so it needs to be general.

there’s at least one case i know where you need the n last: ie some mode that looks at progress over time or if you want to plot the change in some output over time... then you need those checkpoints.

[Ir1d]

@williamFalcon Hi sorry for being absent in the previous days. Should I get #128 ready asap or should I wait for this PR to land?
Should I revert the edits to save_model function in my PR?

[williamFalcon]

@ceyzaguirre4 sorry for the delay. Could you resolve the conflicts so we can merge into this release?

[williamFalcon]

@Ir1d let's get this one merged and you can finish yours based on this.

[Ir1d]

@williamFalcon shall we merge 128 first? since it has been ready since yesterday to catch up the next release

[Borda]

@williamFalcon it is probably only up to you to do the rebase, #320 (comment)
so far I know only PR author (@ceyzaguirre4), repo owner (@williamFalcon) or maintainers have access to the PR to push... :)

[ceyzaguirre4]

@ceyzaguirre4 sorry for the delay. Could you resolve the conflicts so we can merge into this release?

Sorry for being unavailable, I'm presenting a paper at CVPR next week and wont have much time until after then.

---

@williamFalcon shall we merge 128 first? since it has been ready since yesterday to catch up the next release

A: I suggest doing this so you are not held up by my deadlines.

---

I know, but what would it be the case you want to keep n last even they are quite bad?
I understand to keel all metrics abut not bad models... :)

@Borda I find it crucial to at least offer the option to save the very last model. This is so as to reduce as much as possible the negative effects of (for example) an energy outage. Saving the last model is more of an emergency precaution than a legitimate way of selecting the best model.

[ceyzaguirre4]

@williamFalcon in theory the conflicts are solved
However, more tests (those I generally run locally) will have to wait until after the call for papers of CVPR