Skip to content
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

reorganize training.py, code is clearer and handles more corner cases… #7072

Closed
wants to merge 2 commits into from

Conversation

ahundt
Copy link
Contributor

@ahundt ahundt commented Jun 21, 2017

This is the final item in the PR chain necessary to support input tensors. Here is an overview of what will be supported:

# tf yield ops such as those created by the tf `RecordInput()` API
# which supply dataset images and labels
x_train_batch, y_train_batch = read_and_decode_recordinput(...)

# create a basic cnn
x_train_input = Input(tensor=x_train_batch)
x_train_out = cnn_layers(x_train_input)

# y label batch is input & output
# Perhaps this aspect of API usage can be improved?
y_train_in = Input(tensor=y_train_batch)

# ==bugfix==
# This call causes a crash without this patch because
# an invalid call is made that is equivalent to:
# K.placeholder(dtype=x_train_input)
train_model = Model(inputs=[x_train_in], outputs=[x_train_out])

# ==bugfix==
# This call will crash without this patch because
# it is assumed the parameters `x` and `y` are
# provided here and not via the ops
# x_train_batch and y_train_batch 
train_model.compile(optimizer='rmsprop',
                    loss='categorical_crossentropy',
                    metrics=['accuracy'])

# ==bugfix== + ==new param==
# This call will crash without this patch because
# the changes in tensor order caused by the
# constructor update, which accepts yield ops,
# were not previously accounted for.
#
# A new param steps_per_epoch is added
# which works just like in fit_generator()
train_model.fit(None, y_train_in, 
                batch_size=batch_size,
                epochs=epochs,
                steps_per_epoch=10000)

The combined version of these changes (with more detail) is in #6928.

Changes to class Model(Container):

  • relocate _prepare_sample_weights() to be a distinct function
  • define _make_ins() so backend Function() params are created consistently
  • add additional checks to None in many places
  • raise errors in additional exceptional situations
  • add _make_function() so all utilities that create an instance of backend.Function() behave consistently (modulo deliberate / correct differences)

Depends on:
#7067, #7066

Additional fixes in:
#7064, #7067, #7068, #7069, #7071, #7072

Tested by:
#7061

Also see the comments by @Dref360 on the equivalent changes. #7046 (review)

Note: travis failure will be resolved once dependencies above are merged

@ahundt ahundt mentioned this pull request Jun 21, 2017
@ahundt
Copy link
Contributor Author

ahundt commented Jun 26, 2017

@fchollet can we take the next steps on this PR and its dependencies? They are valid error detection and reporting improvements independent of the API Design Review #7102 since they leave the API unchanged.

Here is a good order to look at them that accounts for deps:

  1. training.py _standardize_input_data() cleanup input tensor corner cases #7067 _standardize_input_data() support non-numpy tensor inputs, clear error messages for unsupported inputs, and unit test
  2. training.py _check_loss_and_target_compatibility() fix crash if y is None #7071 training.py _check_loss_and_target_compatibility()
    • bugfix: don't access y param of model.fit() if it is None
  3. Tensorflow backend Function class __init__() and __call__() error handling + fetches + feed_dict #7064 TensorFlow backend Function class init() and call() error handling
  4. is_placeholder() implemented #7066 K.is_placeholder(x) implemented
    • Returns whether x contains a K.placeholder()
  5. model.fit(steps_per_epoch), mnist_tfrecord.py, progbar np.mean #7113 model.fit(steps_per_epoch) added
  6. reorganize training.py, code is clearer and handles more corner cases… #7072 reorganize training.py, code is clearer and handles more corner cases
  7. Yield op tests #7061 yield op unit tests

ahundt added a commit to ahundt/keras that referenced this pull request Aug 5, 2017
* 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)
ahundt added a commit to ahundt/keras that referenced this pull request Aug 5, 2017
* fit_steps_per_epoch:
  removed extraneous line
  mnist_tfrecord.py add coordinator
  mnist_tfrecord.py fix key missing lines
  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)

# Conflicts:
#	examples/mnist_tfrecord.py
fchollet pushed a commit that referenced this pull request Aug 11, 2017
* generic_utils.py don't crash when dealing with batched data

* Progbar() unit test

* mnist_tfrecord.py added (#7061, #7072, #6928, #7046)

* Fix mnist_tfrecord.py runtime errors

* mnist_tfrecord.py pep8

* mnist_tfrecord.py add parallelism option

* reorder inputs

* mnist_tfrecord.py indentation fix

* lower batch size and epochs

* loss defaults to None in compile()

* mnist_tfrecord.py

* model.fit(steps_per_epoch) added

* added _check_num_samples for cases when batch_size does not apply

* fix test failures

* remove inaccurate warning

* improved fit(steps_per_epoch) with separate internal epoch loop in _fit_loop

* fit(steps_per_epoch) initial validation support

* training.py pep8

* mnist_tfrecord.py fix key missing lines

* mnist_tfrecord.py add coordinator

* removed extraneous line

* mnist_tfrecord.py and training.py clean up based on review (#7113)

* mnist_tfrecord.py extended description

* training.py fix test error

* mnist_tfrecord.py and training.py fixed review comments, docs, and error messages (#7113)

* training.py fix unit test error for steps_per_epoch

* fix docstring comments from review (#7113)

* training.py improve docstrings and error case
@stale
Copy link

stale bot commented Oct 24, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 30 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the stale label Oct 24, 2017
@ahundt
Copy link
Contributor Author

ahundt commented Oct 31, 2017

This would need to be redone due to changes on master and I've run out of cycles to keep it up to date, closing for now. If others are interested in incorporating similar changes please let me know and I'll be happy to help.

@ahundt ahundt closed this Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant