Skip to content
This repository has been archived by the owner on Oct 17, 2021. It is now read-only.

Add EarlyStopping callback #518

Merged
merged 8 commits into from
Apr 6, 2019
Merged

Conversation

caisq
Copy link
Contributor

@caisq caisq commented Apr 3, 2019

FEATURE

Fixes tensorflow/tfjs#1087

This change is Reviewable

@caisq caisq requested a review from bileschi April 4, 2019 04:09
Copy link
Contributor

@bileschi bileschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @bileschi and @caisq)


src/callbacks.ts, line 144 at r1 (raw file):

Quoted 6 lines of code…
      if (this.monitor.indexOf('acc') !== -1) {
        this.monitorFunc = greater;
      } else {
        this.monitorFunc = less;
      }
    }

maybe note here this is the 'auto' functionality.


src/callbacks.ts, line 183 at r1 (raw file):

restorBestWeights

nit (sp) restore


src/callbacks_test.ts, line 30 at r1 (raw file):

await callback.onEpochBegin(0);

Does this do anything? it looks like it should call Callback.onEpochBegin() -> BaseCallback.onEpochBegin() ... which seems to be a noop.

Is this included for future proofing?


src/callbacks_test.ts, line 158 at r1 (raw file):

minDelta: -5}

This is the same configuration as minDelta: 5 right?
Is this not confusing? Should we just throw on configured minDelta < 0 ?


src/callbacks_test.ts, line 194 at r1 (raw file):

lears

lears?


src/callbacks_test.ts, line 207 at r1 (raw file):

  // Note that the default monitor (val_loss) is missing.

Great testing code!

Copy link
Contributor Author

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @bileschi)


src/callbacks.ts, line 144 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
      if (this.monitor.indexOf('acc') !== -1) {
        this.monitorFunc = greater;
      } else {
        this.monitorFunc = less;
      }
    }

maybe note here this is the 'auto' functionality.

Done.


src/callbacks.ts, line 183 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
restorBestWeights

nit (sp) restore

Done.


src/callbacks_test.ts, line 30 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
await callback.onEpochBegin(0);

Does this do anything? it looks like it should call Callback.onEpochBegin() -> BaseCallback.onEpochBegin() ... which seems to be a noop.

Is this included for future proofing?

Right. For this particular callback it doesn't do anything. But in the future it may get used for something else.


src/callbacks_test.ts, line 158 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
minDelta: -5}

This is the same configuration as minDelta: 5 right?
Is this not confusing? Should we just throw on configured minDelta < 0 ?

Yes. Keras (Python) allows both positive and negative. I think we should be consistent with that.


src/callbacks_test.ts, line 194 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
lears

lears?

Corrected (should be "leads")


src/callbacks_test.ts, line 207 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
  // Note that the default monitor (val_loss) is missing.

Great testing code!

Ack.

Copy link
Contributor Author

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, @bileschi !

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @bileschi)

@caisq caisq merged commit 45bcffe into tensorflow:master Apr 6, 2019
@caisq caisq deleted the early-stopping-callback branch April 6, 2019 01:10
Copy link
Member

@davidsoergel davidsoergel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @bileschi and @caisq)


src/callbacks.ts, line 31 at r2 (raw file):

}

export interface EarlyStopingCallbackArgs {

typo: Stopping


src/callbacks.ts, line 86 at r2 (raw file):

   * **`True` is not supported yet.**
   */
  restoreBestWeights?: boolean;

I think the question of which weights to keep is orthogonal to early stopping. Sure we should support it, but I think the config belongs somewhere else.


src/callbacks.ts, line 212 at r2 (raw file):

 * quantity has stopped improving.
 *
 * Early stopping is a type of regularization, and protectes model against

typo: protects


src/callbacks.ts, line 215 at r2 (raw file):

 * overfitting.
 *
 * The following example is based on fake data illustrates how this callback

nit: delete 'is', maybe put '(based on fake data)' in parens


src/callbacks.ts, line 239 at r2 (raw file):

 *   epochs: 10,
 *   validationData: [xsVal, ysVal],
 *   callbacks: tf.callbacks.earlyStopping({monitor: 'val_acc', patience: 4})

Since patience: 4, shouldn't training stop after the 4th epoch? Comment above says '2nd', and below says 'length-2'

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants