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

callbacks.early_stopping ignores min_delta and other thoughts #11

Open
JamesAllingham opened this issue Aug 29, 2023 · 1 comment
Open

Comments

@JamesAllingham
Copy link
Contributor

The early_stopping callback doesn't seem to use the min_delta option.

I think that this condition

if (
    self._best is None
    or (self.minimize and value < self._best)
    or (not self.minimize and value > self._best)
):

should be

if (
    self._best is None
    or (self.minimize and value < self._best - self.min_delta)
    or (not self.minimize and value > self._best + self.min_delta)
):

Also, something that confused me slightly is that the best weights are only returned if the patience has run out:

if elapsed - self._elapsed_start >= self.patience:
  if self.restore_best_weights and self._best_state is not None:
      state = self._best_state
  stop_iteration = True

So if you have the best performance on the nth iteration before the end of the training, and you have a patience of p > n, then at the end of the training you will not have the best weights returned, as I understand it. I think this can be solved by adding an on_epoch_end method for the callback. Keras EarlyStopping callback doesn't depend on the patience value for returning the best weights: https://github.com/keras-team/keras/blob/b3ffea6602dbbb481e82312baa24fe657de83e11/keras/callbacks.py#L2088.

Finally, thinking of the Keras features and some others that might be nice, what do you think of having some additional options:

  1. initial_patience (e.g., start_from_epoch from Keras, but allowing any period).
  2. delta_type which is either 'absolute' or 'relative', which specifies how the min_delta is treated. Sometimes it is easier to specify a relative change than an absolute one. Alternatively, we could rename min_delta as min_absolute_delta and introduce min_relative_delta.

Let me know your thoughts, and I'll be happy to take a crack at a PR to address some or all of the above.

@cgarciae
Copy link
Owner

Hey @JamesAllingham, thanks for reporting this!
Yeah, feel free to send a PR, happy to review it :)

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

No branches or pull requests

2 participants