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

Two improvements to Model.fit() #288

Merged
merged 7 commits into from
Aug 13, 2018
Merged

Two improvements to Model.fit() #288

merged 7 commits into from
Aug 13, 2018

Conversation

caisq
Copy link
Contributor

@caisq caisq commented Aug 11, 2018 •
edited

FEATURE

  1. Make Model.fit() (including Sequential.fit()) is called with another fit() call ongoing, an error
    will immediately be thrown.
  2. Make force stopping of fit() calls through stopTraining = true work from not only class objects,
    but also non-class object function callbacks.

Also:

  • Add doc string and code snippet to the stopTraining setters.

Fixes: tensorflow/tfjs#558

This change is Reviewable

bileschi
bileschi approved these changes Aug 13, 2018
View reviewed changes
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.

LGTM just some small comments and questions.

Reviewed 4 of 4 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq, @davidsoergel, @ericdnielsen, and @bileschi)

src/models_test.ts, line 1148 at r1 (raw file):

Do we know why we're still getting style changes mixed in with our functional changes? Is there something we can do with our lint config?

src/models_test.ts, line 1210 at r1 (raw file):

      useBias: false,
      kernelInitializer: 'ones',

not necessary to include the bias and initializer

src/models_test.ts, line 1217 at r1 (raw file):

// Do not call `await` below, so the two fit() calls may interleave.

Under what circumstances would this still complete before the next call?

src/models_test.ts, line 1243 at r1 (raw file):

 useBias: false, kernelInitializer: 'ones'

bias and initializer configuration not necessary.

src/engine/training_test.ts, line 549 at r1 (raw file):

 model.fit(inputs, targets, {batchSize: numSamples, epochs: 8});

This test will fail if the first model.fit completes before the second call. Is this at all likely to happen on some architectures? Can it be avoided?

bileschi
bileschi reviewed Aug 13, 2018
View reviewed changes
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.

:lgtm_strong:

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

caisq
caisq commented Aug 13, 2018
View reviewed changes
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 @caisq, @bileschi, @davidsoergel, and @ericdnielsen)

src/models_test.ts, line 1148 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…

Do we know why we're still getting style changes mixed in with our functional changes? Is there something we can do with our lint config?

I think this has to do with different versions of clang-format being used...

src/models_test.ts, line 1210 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
      useBias: false,
      kernelInitializer: 'ones',

not necessary to include the bias and initializer

True. Done.

src/models_test.ts, line 1217 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
// Do not call `await` below, so the two fit() calls may interleave.

Under what circumstances would this still complete before the next call?

If you call await, the model.fit() call in the line below will complete before the following line.
If you don't call await, it is still theoretically possible for this call to complete before the next call starts, but because the large number of epochs specified here, that is highly unlikely.

src/models_test.ts, line 1243 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
 useBias: false, kernelInitializer: 'ones'

bias and initializer configuration not necessary.

Done.

src/engine/training_test.ts, line 549 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
 model.fit(inputs, targets, {batchSize: numSamples, epochs: 8});

This test will fail if the first model.fit completes before the second call. Is this at all likely to happen on some architectures? Can it be avoided?

As I wrote in the response above, it is only possible if model.fit() completes really fast. But that is unlikely given the large number of epochs used here. I can't think of an alternative way to force the two model.fit() calls to interleave. JavaScript is not multi-threaded after all! I haven't seen this test fail after running it about a dozen times on different devices (including the ones on Travis).

@bileschi
Copy link
Contributor

bileschi commented Aug 13, 2018 via email

@caisq caisq merged commit e07920f into tensorflow:master Aug 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bileschi bileschi bileschi approved these changes

@davidsoergel davidsoergel Awaiting requested review from davidsoergel

@ericdnielsen ericdnielsen Awaiting requested review from ericdnielsen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@caisq @bileschi