-
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
Bug Fix in Image Generator - Now Allows FCN's to be used #5228
Conversation
Made minor changes to allow for FCN's: - Target Size is now a tuple of (None, None) as only this can propagate through the directory iterator - Altered batch_x to be created in the loop, as you should only use a batch size of 1 for FCN of shape (None, None, Channels) - Allowed the directory iterator to use the folder it is passed if it finds no subfolders for classes (Usual use case in FCN) Allowed BGR to be specified (for when using caffe converted models).
PEP8 :|
@@ -191,7 +191,7 @@ def flip_axis(x, axis): | |||
return x | |||
|
|||
|
|||
def array_to_img(x, dim_ordering='default', scale=True): | |||
def array_to_img(x, dim_ordering='default', scale=True, color_mode='rgb'): |
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.
If you add an argument you should document it in the docstring.
@@ -241,7 +243,7 @@ def array_to_img(x, dim_ordering='default', scale=True): | |||
raise ValueError('Unsupported channel number: ', x.shape[2]) | |||
|
|||
|
|||
def img_to_array(img, dim_ordering='default'): | |||
def img_to_array(img, dim_ordering='default', color_mode='rgb'): |
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.
Same.
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.
Fixed both, please tell me if the docstring doesn't suffice.
Added docstrings
@joeyearsley I'd appreciate an example. Also, https://github.com/aurora95/Keras-FCN has some good scripts for training FCNs |
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.
Sorry for the long delay. I feel like I do not fully understand what you are trying to do, maybe because I don't fully understand what you mean by 'FCN'.
My definition of 'FCN' is a fully-convolutional network, i.e. a network that only includes convolutional layers (in particular no final classifier). This makes not assumption about targets.
@@ -298,7 +308,7 @@ def load_img(path, grayscale=False, target_size=None): | |||
img = img.convert('L') | |||
else: # Ensure 3 channel even when loaded image is grayscale | |||
img = img.convert('RGB') | |||
if target_size: | |||
if target_size[0] and target_size[1]: # Can't pass None in from iterator, hence tuple |
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 can keep the default to None
and convert to (None, None)
here. At the very least, target_size=None
should keep being supported (for backwards compatibility).
@@ -777,6 +787,9 @@ def __init__(self, directory, image_data_generator, | |||
for subdir in sorted(os.listdir(directory)): | |||
if os.path.isdir(os.path.join(directory, subdir)): | |||
classes.append(subdir) | |||
# User passed in None and no sub-folders, hence FCN. N.B.: Pass in classes=['.'] for improvements. |
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 cannot quite understand this comment. Can you clarify?
Also: line too long.
@fchollet I believe by FCN he means support for dense prediction tasks like image segmentation of which the FCN paper happens to be one implementation, right or not the two terms are often used interchangeably. This class of tasks are not yet well supported by Keras in the "best practices by default" sense in which Keras is designed. A number of implementations have popped up to handle the problem. Examples of missing keras capabilities required for best practice dense prediction include:
It would be nice if support for dense prediction could be added to the requests for contributions so dense prediction improvements would qualify for direct inclusion in keras, rather than going through keras-contrib. |
@ahundt would you like to take over that project? It would involve:
Yes, we can add to the 'request for contributions' section. |
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.
have a couple questions & a padding issue that should be handled
batch_x = np.zeros((current_batch_size,) + self.image_shape) | ||
# If image_shape has None in position 1 it is intended to be used as a FCN | ||
if self.image_shape[1]: | ||
batch_x = np.zeros((current_batch_size,) + self.image_shape) |
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.
does this assume all the images have the same shape?
If so, varying image dimensions should be handled, and some kind of target dimensions could be utilized to apply padding to the images so they are all the same size.
reference code:
https://github.com/aurora95/Keras-FCN/blob/370eb767df86f364d93eff9068a3edb0b194a18b/utils/SegDataGenerator.py#L221
https://github.com/nicolov/segmentation_keras/blob/master/utils/image_reader.py#L79
x = img_to_array(img, dim_ordering=self.dim_ordering, color_mode=self.color_mode) | ||
# Allows shape to be None, but batch size must equal one. | ||
if not self.image_shape[1] and i == 0: | ||
if current_batch_size != 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.
Why must batch size be one? Just looking for clarification because I'm not 100% sure what this case is checking for because dense prediction tasks including FCNs can train with batch sizes >1.
@@ -160,6 +160,33 @@ def test_directory_iterator(self): | |||
assert(sorted(dir_iterator.filenames) == sorted(filenames)) | |||
shutil.rmtree(tmp_folder) | |||
|
|||
''' Test for the directory iterator when we wish to use it for a FCN.''' | |||
def test_directory_iterator_fcn(self): |
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 suggest also checking if labels can be loaded from pngs without palette conversion as described in the following, perhaps an additonal color mode 'segmentation_label' would be appropriate:
nicolov/segmentation_keras#14
Though perhaps something like that is out of scope for this pull request.
@fchollet sure I can give organizing that project a shot, though it may depend on how busy my research gets this summer. The bullets from my previous comment are a start. Where should this discussion continue so this pull request doesn't need to be hijacked any more than I've already done? |
@ahundt please open an issue with the list of changes you would like to see, and we will create a 'request for contributions' entry that will link to the issue. When you have an API proposal ready for review, please ping me. |
I needed FCN's so have altered this preprocessing step, I can also make a gist for the tutorials if anyone else prefers to learn through a simple example.
This does not allow for no sizes to be set if max poolings are used. In that case, the sizes can be none-square but have to be multiple of 2^m - where m is the number of max pooling layers.
In the future it might be possible to auto-resize to the nearest factor, but another variable will need adding to ensure we can disentangle the model from the preprocessing.
Made minor changes to allow for FCN's:
Allowed BGR to be specified (for when using caffe converted models).