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

Activate initialEpoch for model.fit #261

Merged
merged 7 commits into from
Jul 10, 2018
Merged

Conversation

hpssjellis
Copy link
Contributor

@hpssjellis hpssjellis commented Jul 7, 2018

See feature request to activate initialEpoch at

tensorflow/tfjs#497


This change is Reviewable

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.

Thanks for your PR! Do you think you could add a test for this behavior, possibly by cloning and modifying the test in 'training_test' named '1 input, 1 output, dense, 1 weight, string optimizer, 2 epochs'?

Reviewable status: 0 of 1 LGTMs obtained

@hpssjellis
Copy link
Contributor Author

hpssjellis commented Jul 9, 2018

@bileschi

Tested on a ubuntu laptop. If you look at the training_test.ts code here

For a 2 epoch run by setting initialEpoch to 1 then the history.epoch variable should be [1] instead of [0,1]

Copy link
Contributor

@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: 0 of 1 LGTMs obtained


src/engine/training.ts, line 1311 at r2 (raw file):

?: number

Why is it necessary to change = 0 to ?: number here? I don't think this change is necessary.


src/engine/training_test.ts, line 521 at r2 (raw file):

 expect(history.epoch).toEqual([1]);

Can we test this more thoroughly by making an assertion on history.history.loss? Something like:

expect(history.history.loss.length).toEqual(1);

@hpssjellis
Copy link
Contributor Author

@bileschi

Quote from @shanqing-cai

initialEpoch?: number
Why is it necessary to change = 0 to ?: number here? I don't think this change is necessary.

end of quote.

I can make the requested changes, I was just trying to make initialEpoch follow the same format as: epochs, batchSize and shuffle, since the following code already covers the default settings.

 if (batchSize == null) {
      batchSize = 32;
    }
    if (epochs == null) {
      epochs = 1;
    }
    if (shuffle == null) {
      shuffle = true;
    }
    if (initialEpoch == null) {
      initialEpoch = 0;
    }

P.S. I notice the above code does not detect incorrect negative inputs, would the following code be an improvement?

    if ((batchSize == null) || (batchSize < 0)) {
      batchSize = 32;
    }
    if ((epochs == null) || (epochs < 0)) {
      epochs = 1;
    }
    if (shuffle == null) {
      shuffle = true;
    }
    if ((initialEpoch == null) || (initialEpoch < 0)) {
      initialEpoch = 0;
    }

Either way I am fine with whatever you think is best, just thought I would explain my thinking with the changes.

Copy link
Contributor

@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.

Ah - that works. So if you can address my other comment about adding some more testing assertions, I will approve this PR.

Thanks.

Reviewable status: 0 of 1 LGTMs obtained

Copy link
Contributor

@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.

:lgtm_strong:

Thanks for this, @hpssjellis !

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

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! 2 of 1 LGTMs obtained

@caisq caisq merged commit 436fe31 into tensorflow:master Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants