-
Notifications
You must be signed in to change notification settings - Fork 19.5k
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
model.fit(steps_per_epoch), mnist_tfrecord.py, progbar np.mean #7113
Conversation
keras/engine/training.py
Outdated
if ins and hasattr(ins[0], 'shape'): | ||
num_train_samples = ins[0].shape[0] | ||
if steps_per_epoch is not None: | ||
num_train_samples = steps_per_epoch |
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.
num_train_samples != steps_per_epoch, though. A step is a batch, not a single sample.
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.
Ah yes forgot to divide but batch size. I'll do that when I'm at a computer
I think what we should do is:
There are serious issues with blending both step-based counting and sample-based counting (the last batch, variable batch sizes, etc). So we can't mix them without complications. |
I agree with your suggestion. I just made a small tweak since the |
Seems it is necessary to allow both to be specified because batch_size is not ignored, it is used in some of the totals and other calculations so I'm not sure the warning is appropriate without more substantial internal changes. Example error:
|
keras/engine/training.py
Outdated
@@ -962,10 +962,21 @@ def _make_predict_function(self): | |||
name='predict_function', | |||
**kwargs) | |||
|
|||
def _check_num_samples(self, ins, batch_size=None, steps=None, steps_name='steps'): | |||
if steps is not None: | |||
num_samples = steps |
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.
That should be, on average, step * batch_size
, not steps
. However, since the data length may not be a multiple of batch_size
, and because in the generator case there may be batches of different sizes, it is actually impossible to determine num_samples
.
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.
Perhaps something should be renamed in this case or documentation modified a bit for everything to be consistent?
Also, I wasn't sure if you're saying I should make it step*batch_size
here or if you're saying that's also inaccurate and we need to do something else.
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.
We need to separate two use cases cleanly:
- training for a specific number of steps (if the data has no length), like in
fit_generator
. - training for a specific number of samples (if the data has a length), like in current
fit
.
We can't unify both because no reliable conversion exists between the two.
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.
Would the best approach then be the creation of a second code path in fit()
that works more like fit_generator()
? Perhaps it would be best to create a new function like fit_tensor()
?
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.
Good question. Can we make the expanded API work with fit
without refactoring or adding new methods?
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.
It should be pretty simple to do with the same methods just by checking if either x
or y
is a Keras tensor in fit(x, y, ...)
, so long as steps_per_epoch
and epochs
from fit_generator()
are added to fit(x, y, ..., steps_per_epoch=None, epochs=None)
.
Unless you have another suggestion, I propose checking for Keras tensors because it is the simplest mechanism by which multiple platforms, not just TF, could eventually support the API. This means wrapping TF tensors with Input()
, as in #6928 or see the more atomic PR merge order outlined in #7072 (comment).
…s_per_epoch * commit '01e2148732e4083b4850345e5ce4dd499cb5999e': (98 commits) Fix common backend styles (keras-team#7476) Allow dynamic shape for repeat_elements (keras-team#7422) Fix test style (keras-team#7473) Crossentropy backend API consistency cleanup (keras-team#7199) Replace the reserved word `input` with `inputs` (keras-team#7474) Increase test coverage (keras-team#7264) Fix l2_normalize Add default value for `l2_normalize`. Remove legacy axis handling in TF backend. Update save_model function (keras-team#7455) Switch to pydot 1.2.x (keras-team#7448) Add save, evaluate and predict functionality for example (keras-team#7430) Docs fix: `pointwise` instead of `depthwise` for SeparableConv2D (keras-team#7444) Fix conv reccurent test Style fix in conv recurrent tests. Support return_state parameter in ConvRecurrent2D (keras-team#7407) Small simplification in ResNet50 architecture Update FAQ with info about custom object loading. add example for passing in custom objects in load_model (keras-team#7420) Update applications.md (keras-team#7428) ...
* mnist_tfrecord: mnist_tfrecord.py loss defaults to None in compile() lower batch size and epochs mnist_tfrecord.py indentation fix mnist_tfrecord.py add parallelism option mnist_tfrecord.py pep8 Fix mnist_tfrecord.py runtime errors mnist_tfrecord.py added (keras-team#7061, keras-team#7072, keras-team#6928, keras-team#7046)
Large batch sizes can run much in less wall clock time and still get better performance. For example |
@fchollet hopefully everything should be addressed now |
* fit_steps_per_epoch: training.py fix unit test error for steps_per_epoch mnist_tfrecord.py and training.py fixed review comments, docs, and error messages (keras-team#7113) training.py fix test error mnist_tfrecord.py extended description mnist_tfrecord.py and training.py clean up based on review (keras-team#7113) # Conflicts: # examples/mnist_tfrecord.py
initial_epoch=initial_epoch) | ||
initial_epoch=initial_epoch, | ||
steps_per_epoch=steps_per_epoch, | ||
validation_steps=validation_steps) | ||
|
||
def evaluate(self, x, y, batch_size=32, verbose=1, sample_weight=None): |
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.
Do you want to add the tensor-input support for evaluate
immediately as well, or do that in another PR?
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.
Good point. Let's add data tensor support to evaluate
and predict
in this CL.
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'd prefer to hold off and get these changes in. We can cherry pick changes from #6928 in a separate PR for that if you think it is a good idea.
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.
Ah the most recent post didn't load, also I had changed my mind as well and added evaluate since it was only a couple lines. I'll look at predict too.
@@ -1513,8 +1611,8 @@ def predict(self, x, batch_size=32, verbose=0): | |||
ins = x | |||
self._make_predict_function() | |||
f = self.predict_function | |||
return self._predict_loop(f, ins, | |||
batch_size=batch_size, verbose=verbose) | |||
return self._predict_loop(f, ins, batch_size=batch_size, |
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.
leave as-is?
@@ -734,7 +739,7 @@ def test_model_with_external_loss(): | |||
out = model.predict_on_batch(None) | |||
|
|||
# test fit | |||
out = model.fit(None, None, epochs=1, batch_size=10) | |||
out = model.fit(None, None, epochs=1, batch_size=None, steps_per_epoch=1) |
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 verified. Looks great!
LGTM |
keras/engine/training.py
Outdated
@@ -1154,58 +1222,70 @@ def _predict_loop(self, f, ins, batch_size=32, verbose=0): | |||
return outs[0] | |||
return outs | |||
|
|||
def _test_loop(self, f, ins, batch_size=32, verbose=0): | |||
def _test_loop(self, f, ins, batch_size=None, verbose=0, steps=None): | |||
"""Abstract method to loop over some data in batches. | |||
|
|||
# Arguments | |||
f: Keras function returning a list of tensors. | |||
ins: list of tensors to be fed to `f`. | |||
batch_size: integer batch size. |
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.
or None
for l, o in zip(out_labels, val_outs): | ||
epoch_logs['val_' + l] = o | ||
else: | ||
if shuffle == 'batch': |
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.
Maybe add a warning that if steps_per_epoch is not None
, shuffle=True
has no effect. (no test required since pytest is flaky when it catches warning)
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.
adding a comment to the docs, I'd rather not see that warning all the time by default
epochs=1, batch_size=4, validation_split=0.5) | ||
epochs=1, batch_size=None, validation_split=0.5, | ||
steps_per_epoch=1) | ||
out = model.fit({'input_a': input_a_np, 'input_b': input_b_np}, |
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.
predict
and evaluate
with data tensors should have unit tests as well.
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.
see #7113 (comment)
test_model.compile(optimizer='rmsprop', loss='categorical_crossentropy', metrics=['accuracy']) | ||
test_model.summary() | ||
|
||
loss, acc = test_model.evaluate(x_test, np_utils.to_categorical(y_test), classes) |
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.
Could we use evaluate
with a data tensor in this example?
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 just realized we've started along the path to reimplementing #6928, and would require changes that separate numpy arrays from tensors. That PR allows the labels y
to be passed as a tensor. For that reason I think we should revert support for evaluate(steps)
and predict(steps)
, fix the docstring issues, and limit this PR to fit()
.
Full support can be integrated with what we called API 3 via #6928.
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.
We already have support for evaluate
from data tensors. https://github.com/fchollet/keras/blob/master/tests/keras/engine/test_training.py#L740
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.e. all we need is add steps
to these methods)
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.
The simplicity of the test hides the lack of underlying implementation.
In this test there is no label y
to evaluate against. You'll see in the _test_loop()
the output of f(ins) is not saved in the steps
case, which is ok for this PR as it was originally conceived.
This seemingly simple missing component actually requires some implementation to work. I'm not sure if supplying a Tensor as the parameter y
will give results in the format described by #Returns
. Perhaps simply retaining a list of the outputs is OK like in _step_loop()? (the link is to the #6928 version of the changes I reverted here)
I'm not sure about indexing correctly to calculate the summaries for each output in this case, e.g. something equivalent to outs[i][batch_start:batch_end] = batch_out
in _predict_loop()
or in _test_loop
there is outs[i] += batch_out * len(batch_ids)
and then outs[i] /= samples
.
3e13d6c
to
56736f3
Compare
I think it's preferable to have them here for API consistency. Like in |
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.
Discussing the requirements of predict(steps)
and evaluate(steps)
below. Perhaps I misunderstood and the accuracy is calculated somewhere in the mnist_tfrecord.py
call to fit()
?
I believe only loss is calculated and no accuracy data is available.
if verbose == 1: | ||
progbar = Progbar(target=steps) | ||
for step_num in range(steps): | ||
f(ins) |
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.
Nothing is saved out in this case. Can the results be collated in a predictable manner? I'm not yet sure if the answer may depend on the input tensor the user supplies, such as RecordInput
vs tf.train.shuffle_batch
.
test_model.compile(optimizer='rmsprop', loss='categorical_crossentropy', metrics=['accuracy']) | ||
test_model.summary() | ||
|
||
loss, acc = test_model.evaluate(x_test, np_utils.to_categorical(y_test), classes) |
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.
The simplicity of the test hides the lack of underlying implementation.
In this test there is no label y
to evaluate against. You'll see in the _test_loop()
the output of f(ins) is not saved in the steps
case, which is ok for this PR as it was originally conceived.
This seemingly simple missing component actually requires some implementation to work. I'm not sure if supplying a Tensor as the parameter y
will give results in the format described by #Returns
. Perhaps simply retaining a list of the outputs is OK like in _step_loop()? (the link is to the #6928 version of the changes I reverted here)
I'm not sure about indexing correctly to calculate the summaries for each output in this case, e.g. something equivalent to outs[i][batch_start:batch_end] = batch_out
in _predict_loop()
or in _test_loop
there is outs[i] += batch_out * len(batch_ids)
and then outs[i] /= samples
.
@fchollet @TimZaman @Dref360 Considering the need for predict & evaluate, which then requires labels from input tensors, could we simply continue the review over at #6928, or is there a better approach? Everything from this PR #7113 is merged into #6928. # API 2
model = # on top of the tensor input
model.add_loss() # involving y_tensor
model.fit(epochs=10, steps_per_epoch=1000)
# API 3
model = # on top of the tensor input
model.compile()
model.fit(y=y_tensor, epochs=10, steps_per_epoch=1000) There are extended unit tests and support for Input Tensors with each of the following APIs: # train_on_batch
out = model.train_on_batch(x, y)
# test_on_batch
out = model.test_on_batch(x, y)
# predict_on_batch
out = model.predict_on_batch(x)
# fit
out = model.fit(x, y, epochs=1, batch_size=batch_size,
steps_per_epoch=steps_per_epoch)
# evaluate
out = model.evaluate(x, y, batch_size=batch_size,
steps=steps_per_epoch)
# predict
out = model.predict(x, batch_size=batch_size,
steps=steps_per_epoch) I've also incorporated and merged all feedback from all the split atomic PRs into #6928, so it is the canonical version with the most complete functionality for the purposes discussed here, but I'm ok with an alternative solution that works as well. :-) |
keras/engine/training.py
Outdated
steps: Total number of steps (batches of samples) | ||
before declaring _predict_loop finished. | ||
Ignored with the default value of `None`. | ||
steps_name: The public API's parameter name for `steps`. |
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.
Docstring should have a Returns
section and a Raises
section.
keras/engine/training.py
Outdated
ins: list of tensors to be fed to the Keras function. | ||
batch_size: integer batch size or None if unknown. | ||
steps: Total number of steps (batches of samples) | ||
before declaring _predict_loop finished. |
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.
Using code markers around code keywords (.e.g _predict_loop
).
keras/engine/training.py
Outdated
if hasattr(ins[0], 'shape'): | ||
validation_steps = steps_per_epoch | ||
else: | ||
raise ValueError('When `steps_per_epoch` validation ' |
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.
"When using steps_per_epoch
, ..."
keras/engine/training.py
Outdated
|
||
self.history = cbks.History() | ||
callbacks = [cbks.BaseLogger()] + (callbacks or []) + [self.history] | ||
if verbose: | ||
callbacks += [cbks.ProgbarLogger()] | ||
if steps_per_epoch: |
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.
Prefer if steps_per_epoch is not None
@fchollet Addressed the review items here. |
* commit '41fdd5aa5fe080bf5fa12638b9abc9f50d835a39': training.py improve docstrings and error case fix docstring comments from review (keras-team#7113)
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!
Awesome!
…On Fri, 11 Aug 2017 at 21:06, François Chollet ***@***.***> wrote:
Merged #7113 <#7113>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7113 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHXSRHYCC8oRgAkkvHtCAlJBmlVx9tqXks5sXJgigaJpZM4OENzV>
.
|
* master: model.fit(steps_per_epoch), mnist_tfrecord.py, progbar np.mean (keras-team#7113) Change default size to allow different MobileNet sizes (keras-team#7586) cntk backend: fix the reversed rnn bug (keras-team#7593) Fix mask for multi output --> multi inputs (keras-team#7591) [RELNOTES] Subtract merge layer (keras-team#7573) update docker with cntk 2.1 (keras-team#7565) [RELNOTES] Move constraint management to be based on variable attributes (like TF). (keras-team#7564) Add handling for `dtype` arg in initializer config. Fix keras-team#7550. (keras-team#7552) remove unnecessary function definition (keras-team#7542) refactor the function - _convert_string_dtype (keras-team#7540) Add batchnorm tests for CNTK (keras-team#7534) Clean up RNN tests (keras-team#7529) Add merge tests (keras-team#7533) # Conflicts: # examples/mnist_tfrecord.py # keras/engine/training.py # keras/utils/generic_utils.py # tests/keras/engine/test_training.py
Yay! |
Implements external loss based support for
model.fit(steps_per_epoch)
with tensor input and a corresponding tfrecord example.Runtime results when running
mnist_tfrecord.py
from python tf tensors:The progbar change is needed for
mnist_tfrecord.py
where a step is a batch.