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

Change default size to allow different MobileNet sizes #7586

Merged
merged 2 commits into from
Aug 11, 2017
Merged

Change default size to allow different MobileNet sizes #7586

merged 2 commits into from
Aug 11, 2017

Conversation

titu1994
Copy link
Contributor

@titu1994 titu1994 commented Aug 10, 2017

@fchollet There is a MobileNet image size selection problem in the current implementation.

Due to the use of _obtain_input_shape(...), the default_size provided is a static 224. While this generally works well for models with only 1 input size, for the MobileNet family of models, it is not suitable.

The issue is that when one passes input_shape as anything other than the default of 224 (128, 160 and 192 are supported as well), an error stating that the default size required with include_top=True is (224, 224, 3) is raised. This is a problem, since there exist weights for smaller input size models. They just can't be accessed with the standard working of _obtain_input_shape.

There are two possible ways to fix this :

  1. Make _obtain_input_shape support a list of default sizes. Usage of this method would be simpler then, though a very clean implementation would be a tad bit more difficult (but can be managed).

  2. Do a pre-check for input_shape (if provided), check it is one of the possible sizes supported by MobileNet, and then change the default_size to match this input size. There is a fallback to the default_size = 224 if the provided input shape is not in the list of supported sizes (and then _obtain_input_shape can check and raise errors if needed).
    This means there is no need to change the _obtain_input_shape code, and the task falls to the method caller to make sure the correct default_size is provided.

In my opinion, since it is relatively rare for a single model to be trained on different image sizes, I prefer option 2. Therefore, that has been implemented in this PR.

@titu1994 titu1994 changed the title Change default size to allow different MobileNet variant sizes Change default size to allow different MobileNet sizes Aug 10, 2017
@titu1994 titu1994 closed this Aug 10, 2017
@titu1994 titu1994 reopened this Aug 10, 2017
@titu1994
Copy link
Contributor Author

titu1994 commented Aug 10, 2017

Rerunning tests, since it says it passed everything but still shows an error.

@fchollet
Copy link
Collaborator

There is a MobileNet image size selection problem in the current implementation.

Please add a unit test.

Don't worry about the CI failure, Travis is currently having some issues.

@titu1994
Copy link
Contributor Author

titu1994 commented Aug 10, 2017

@fchollet Added a unit test to check whether the 4 valid input shapes models are correctly built and an invalid image size test to make sure a ValueError is raised from _obtain_input_shape is properly raised.

@fchollet fchollet merged commit 1e9ee7e into keras-team:master Aug 11, 2017
ahundt added a commit to ahundt/keras that referenced this pull request Aug 11, 2017
* 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
@titu1994 titu1994 deleted the mobilenet branch August 12, 2017 16:25
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.

2 participants