-
Notifications
You must be signed in to change notification settings - Fork 19.4k
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
Sync Sequential.fit() with Model.fit() #8192
Conversation
...instead of relying on implicit default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe not all proposed docstring edits are warranted. Please put them in a different PR, to make it easier to review and merge.
Ok for the API sync.
@@ -247,12 +247,12 @@ def test_EarlyStopping_reuse(): | |||
stopper = callbacks.EarlyStopping(monitor='acc', patience=patience) | |||
weights = model.get_weights() | |||
|
|||
hist = model.fit(data, labels, callbacks=[stopper]) | |||
hist = model.fit(data, labels, callbacks=[stopper], epochs=20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would slow down the tests and should be set to a lower value (e.g. 10). Is this change related to the rest of the PR? Doesn't look like it is...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this change (introduced in the second commit) tests fail (see first commit). Maybe this change is just papering over a real bug?
I chose epochs=20, because earlier in the same function epochs=20 is used. In the whole file, epochs=20 is used 8 times, and epochs=10 is used 0 times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok then.
Put docstrings into a different PR #8233 |
@ozabluda I will take a look, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
* Sync Sequential.fit() with Model.fit() * Specify explicit epochs=20 ...instead of relying on implicit default value. * Revert docstring changes * pep8 * pep8
* Sync Sequential.fit() with Model.fit() * Specify explicit epochs=20 ...instead of relying on implicit default value. * Revert docstring changes * pep8 * pep8
... and improve docstrings.
A couple of API changes, all backward compatible (additions and more permissive), except that now Model.fit(epoch=1) is the default (matching Sequential.fit(). I didn't find any example/*.py affected by this.