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

bilinear upsampling and resizing #9303

Closed
wants to merge 5 commits into from

Conversation

ahundt
Copy link
Contributor

@ahundt ahundt commented Feb 4, 2018

This is an implementation of bilinear interpolation for the backend and layers. It introduces the parameter resize_images(interpolation='bilinear') and UpSampling2D(interpolation='bilinear').

All defaults remain interpolation='nearest' for backwards compatibility.

@ahundt ahundt changed the title bilinear upsampling and resizing initial implementation bilinear upsampling and resizing Feb 4, 2018
output = repeat_elements(output, width_factor, axis=2)
return output
else:
raise ValueError('CNTK Backend: Invalid data_format:', data_format)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe tell the user the valid data_formats

x = tf.image.resize_nearest_neighbor(x, new_shape)
elif interpolation == 'bilinear':
x = tf.image.resize_bilinear(x, new_shape)
else:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this value error is good but do in other places

@@ -884,24 +884,42 @@ def repeat_elements(x, rep, axis):
return y


def resize_images(x, height_factor, width_factor, data_format):
def resize_images(x, height_factor, width_factor, data_format, interpolation='nearest'):
"""Resize the images contained in a 4D tensor of shape
- [batch, channels, height, width] (for 'channels_first' data_format)
- [batch, height, width, channels] (for 'channels_last' data_format)
by a factor of (height_factor, width_factor). Both factors should be
positive integers.
"""
if data_format == 'channels_first':

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comments to make easier to read

@@ -1565,6 +1565,7 @@ class UpSampling2D(Layer):
It defaults to the `image_data_format` value found in your
Keras config file at `~/.keras/keras.json`.
If you never set it, then it will be "channels_last".
interpolation: A string, one of `nearest` or `bilinear`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other doc string stuff has default marked

output = repeat_elements(output, width_factor, axis=2)
return output
else:
raise ValueError('CNTK Backend: Invalid data_format:', data_format)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the tests k.backend() == cntk might want to lower case here

data_format=data_format)
layer.build(inputs.shape)
outputs = layer(K.variable(inputs))
np_output = K.eval(outputs)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this tests need to be more rigorous. only checking shape

@ahundt ahundt closed this Feb 6, 2018
@ahundt ahundt reopened this Feb 6, 2018
@ozabluda
Copy link
Contributor

This is an implementation of bilinear interpolation for the backend and layers. It introduces the parameter resize_images(interpolation='bilinear') and UpSampling2D(interpolation='bilinear').

All defaults remain interpolation='nearest' for backwards compatibility.

default for UpSampling2D has to remain nearest, but maybe after PR #8849, default for resize_images should be bilinear?

BTW, this seems to be how you do it in CNTK:
https://microsoft.github.io/CNTK-R/reference/transform_scale.html

@ahundt
Copy link
Contributor Author

ahundt commented Feb 13, 2018

Can anyone suggest how to fix the theano errors? I'm not really a theano user so I was just implementing my best guess.

ratio = height_factor / width_factor
th_padding = _preprocess_padding('same')
output = theano.tensor.nnet.abstract_conv.bilinear_upsampling(
x, ratio=ratio, border_mode=th_padding)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

border_mode is not required. The output is of value : (batch size, num_input_channels, input row size * row ratio,
input column size * column ratio)

So fi the ratio is good, everything should be.

@bhack
Copy link
Contributor

bhack commented Apr 24, 2018

Any update?

@wtreible
Copy link

I'm new to this, but it seems pretty important that the resizing function should support not only bilinear interpolation, but also bicubic, at least when using the tf backend. Many of my colleagues who have tried Keras are surprised that it only supports nn interpolation, especially since upsampling is used in almost every hourglass-style network.

Also, volume resizing w/ trilinear interp would help Keras keep up with Torch for 3D hourglass implementations.

@bhack
Copy link
Contributor

bhack commented Apr 26, 2018

@wtreible Exactly

@ahundt
Copy link
Contributor Author

ahundt commented Apr 30, 2018

I think getting bilinear merged is step one. Other contributors can add new functionality later.

Maybe I can just leave bilinear unsupported and throwing an error on the one backend and just make the unit tests expect that. Then if someone wants it they can add it later.

@fchollet
Copy link
Collaborator

So, what should we do with this PR? What's the current status? @Dref360 what's your take on it?

@Dref360
Copy link
Contributor

Dref360 commented May 17, 2018

I don't think we should merge it till the Theano version is fixed. The documentation of Theano doesn't match what's happening.

It's a great feature, I just don't think leaving Theano behind TF/CNTK is a good idea.

@ahundt
Copy link
Contributor Author

ahundt commented May 17, 2018

It works correctly on the tf backend, I'm just not sure how to fix the theano error and I'm a bit tight on time to learn new backends so it has been in a holding pattern.

@Dref360 isn't theano no longer being developed? I feel like it can't possibly stay on par forever without ongoing work in the upstream library.

@Dref360
Copy link
Contributor

Dref360 commented May 17, 2018

It's still being supported for at least 4 months. (https://groups.google.com/forum/#!topic/theano-users/7Poq8BZutbY)

My main concern is that it's the first major feature that Theano won't implement.

Currently, the only test Theano is not able to pass is: test_batchnorm_convnet_no_center_no_scale

Should we stop supporting Theano (ie. drop the CI) before October?
We could merge this PR and advise people to switch to another backend.

If we do so, this PR requires little changes (ie. throws if Theano) and the changes for TF are LGTM

@fchollet
Copy link
Collaborator

fchollet commented May 17, 2018

Should we stop supporting Theano (ie. drop the CI) before October?

It's okay if new features we add in Keras aren't supported in Theano. Existing workflows should keep working (and we should keep Theano on CI) for at least 1.5 years from now (Theano EoL + 1 year).

We will gradually drop features when Theano becomes a friction. Whenever it is "cheap" to make things work with Theano, we'll make it work. One recent example is that we dropped recurrent dropout support in Theano because the new implementation couldn't easily be made to work with Theano.

Basically: let's not make Theano a bottleneck of Keras development. But let's not break it when that can be avoided at a low cost.

This PR doesn't change the default behavior, so it would be okay to skip Theano support (won't break existing users).

Copy link
Contributor

@joelthchao joelthchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked why theano backend is failed. Please check my reviews and make it pass theano unit test.

output = repeat_elements(x, height_factor, axis=axis_1)
output = repeat_elements(output, width_factor, axis=axis_2)
elif interpolation == 'bilinear':
ratio = height_factor / width_factor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For theano, ratio needs to be integer.

ratio = height_factor // width_factor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how should this be done? Am I correct that the values might be tensors and not something that can be cast to int on every backend?

repeat_dim_1 = x._keras_shape[axis_1]
repeat_dim_2 = x._keras_shape[axis_2]
output._keras_shape[axis_1] = repeat_dim_1 * rep
output._keras_shape[axis_2] = repeat_dim_2 * rep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unresolved referencerep

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... it has been a while since I looked at this... what was rep supposed to be?

@gabrieldemarmiesse
Copy link
Contributor

I tried to take a stab at it. See #10994.

@ahundt
Copy link
Contributor Author

ahundt commented Aug 26, 2018

oh cool just saw #10994, thanks @gabrieldemarmiesse!

@gabrieldemarmiesse
Copy link
Contributor

@ahundt can you close the PR now that #10994 is merged?

And thanks again for your work! I didn't have to change it much.

@fchollet fchollet closed this Aug 30, 2018
@ahundt ahundt deleted the bilinear_upsampling branch September 27, 2018 02:56
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.

9 participants