Added Convolutional LSTM #8891

loliverhennigh · 2017-04-01T01:29:13Z

Added an implementation of convolutional lstms (https://arxiv.org/abs/1506.04214). Related to this issue #4536 .

tensorflow-jenkins · 2017-04-01T01:29:14Z

Can one of the admins verify this patch?

marcociccone · 2017-04-03T10:55:31Z

I think that if you want to use the cell with dynamic_rnn wrapper, this will not work because it expects a 3D tensor for the scan. Am I right?

ebrevdo · 2017-04-03T15:47:15Z

No; dynamic_rnn supports tensors with shape 2D+ as input.

…

On Mon, Apr 3, 2017 at 3:55 AM, Marco Ciccone ***@***.***> wrote: I think that if you want to use the cell with dynamic_rnn wrapper, this would not work because it needs a 3D tensor for the scan. Am I right? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#8891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABtimzzSM3oi2wehVGpIT1mQmBzoYUbaks5rsNA4gaJpZM4MwTz9> .

marcociccone · 2017-04-03T20:24:20Z

Thanks @ebrevdo, since when it is supported? I'm having an error with my implementation that the shape must be 3D so I'm asking. Do I need to specify any abstract method or something else?

I tag @carlthome because I think he's interested too

ebrevdo · 2017-04-03T23:03:35Z

The tensorflow nightlies should have this bug fixed.

…

On Apr 3, 2017 1:24 PM, "Marco Ciccone" ***@***.***> wrote: Thanks @ebrevdo <https://github.com/ebrevdo>, since when it is supported? I'm having an error with my implementation that the shape must be 3D so I'm asking. Do I need to specify any abstract method or something else? I tag @carlthome <https://github.com/carlthome> because I think he's interested too — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABtim1wXVGdavJIWvCLsI83MMZxSU6uxks5rsVWNgaJpZM4MwTz9> .

marcociccone · 2017-04-04T10:51:09Z

It doesn't seem so, here we have a reshape that fails if I have more than 2 dimensions. Am I missing something? I can open an issue in case I'm right

carlthome · 2017-04-11T12:42:42Z

tensorflow/contrib/rnn/python/ops/rnn_cell.py

+    else:
+      res = nn_ops.conv2d(array_ops.concat(axis=3, values=args), matrix, strides=[1, 1, 1, 1], padding='SAME')
+    if not bias:
+      return res


It's bad style to have multiple returns in the same function.

Rather do if bias: res += bias

I cleaned this up a little bit however the function still has the 2 return statements. The reason I wrote it this way was because the _linear function written in tensorflow/contrib/rnn/python/ops/core_rnn_cell_impl.py has 2 return statements. I can definitely change it but it seemed more consistent to do it this way.

How unfortunate, but consistency 👍

carlthome · 2017-04-11T14:57:06Z

tensorflow/contrib/rnn/python/ops/rnn_cell.py

+      # Parameters of gates are concatenated into one multiply for efficiency.
+      (c, h) = state
+
+      concat = _conv_linear([inputs, h], self._filter_size, self._num_features * 4, True)


If self._num_features is 1, will the gates be created? I think the minimum should be 4 here, no?

I do not believe this is a issue. The gates will still be created. In the tensorflow/contrib/rnn/python/kernel_tests/rnn_cell_test.py test I wrote, the filter size is set to 1 with no issues

carlthome · 2017-04-11T15:05:26Z

tensorflow/contrib/rnn/python/ops/rnn_cell.py

+
+  @property
+  def output_size(self):
+    return self._shape


Shouldn't this include self._num_features?

carlthome · 2017-04-11T15:05:30Z

tensorflow/contrib/rnn/python/ops/rnn_cell.py

+
+  @property
+  def state_size(self):
+    return core_rnn_cell.LSTMStateTuple(self._shape, self._shape)


Shouldn't this include self._num_features?

bhack · 2017-04-24T14:28:23Z

Any update on this review?

carlthome · 2017-04-24T14:46:18Z

I don't think this works with tf.nn.dynamic_rnn.

bhack · 2017-04-28T12:13:06Z

@loliverhennigh Do you plan to come back again on this?

…o feature/conv_lstm_rnn

…to feature/conv_lstm_rnn

loliverhennigh · 2017-05-01T05:10:34Z

I updated some of the issues mention above

vrv · 2017-05-01T23:08:23Z

@carlthome @ebrevdo let us know what the next steps are!

ebrevdo · 2017-05-02T18:52:56Z

Hi @loliverhennigh! Thanks for taking the time to sit down and implement this.

Have you seen sonnet's ConvLSTM module? They have a very nice API and implementation that is sort-of but not 100% identical to yours. Would you be interested in writing a ConvLSTM matching this API / impl?

Note also we've gotten rid of _checked_scope in favor of RNNCell subclassing tf.layers.Layer; so we no longer override call but instead have a "def call(self, inputs, state):"

vrv · 2017-05-04T21:55:40Z

friendly ping for @loliverhennigh

loliverhennigh · 2017-05-05T04:12:10Z

Oh cool, I had not seen sonnet yet. I could definitely rewrite my ConvLstm to match that one. I like that it has support for 1,2, and 3d convs.

ebrevdo · 2017-05-05T14:48:48Z

That would be great! On May 4, 2017 9:12 PM, "Oliver Hennigh" <notifications@github.com> wrote: Oh cool, I had not seen sonnet yet. I could definitely rewrite my ConvLstm to match that one. I like that it has support for 1,2, and 3d convs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABtim8lvf_7MzpKKjLTzcYEg5UDALT8wks5r2qGugaJpZM4MwTz9> .

ebrevdo

Can you add versions of the same tests where the batch size is not known and you feed the inputs and initial state via feed_dict to session.run? This should ensure the code works for batch size a Tensor.

…o feature/conv_lstm_rnn

vrv · 2017-07-24T18:18:25Z

Ping for @loliverhennigh on the last comment about adding tests.

…o feature/conv_lstm_rnn

loliverhennigh · 2017-08-05T00:15:41Z

I think this fixes the variable batch size problem and also allows for variable image sizes. The kernel tests now reflect this. I think this might be all good now

lukaszkaiser

The code looks good to me, thanks! (One could remove the conv_dims or have just 1 class, but the current version looks consistent with our layers, so I don't have any strong opinion on that.)

drpngx · 2017-08-05T21:18:33Z

Jenkins, test this please

…

On Aug 5, 2017 12:57 PM, "Lukasz Kaiser" ***@***.***> wrote: ***@***.**** approved this pull request. The code looks good to me, thanks! (One could remove the conv_dims or have just 1 class, but the current version looks consistent with our layers, so I don't have any strong opinion on that.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8891 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AT_Sbcki11vjQA84KiDDjx0oIr1lBEmsks5sVMkSgaJpZM4MwTz9> .

drpngx · 2017-08-07T15:19:36Z

Jenkins, test this please.

drpngx · 2017-08-07T18:08:25Z

The Linux build is transient, but there is a windows build error:

08:30:35      4>measuring_cost_estimator.obj : error LNK2019: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl tensorflow::SanitizeThreadSuffix(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" (?SanitizeThreadSuffix@tensorflow@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V23@@Z) referenced in function "public: __cdecl tensorflow::grappler::MeasuringCostEstimator::MeasuringCostEstimator(class tensorflow::grappler::Cluster *,int,int)" (??0MeasuringCostEstimator@grappler@tensorflow@@QEAA@PEAVCluster@12@HH@Z) [C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj]
08:30:35      4>queue_runner.obj : error LNK2001: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl tensorflow::SanitizeThreadSuffix(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" (?SanitizeThreadSuffix@tensorflow@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V23@@Z) [C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj]
08:30:35      4>single_machine.obj : error LNK2001: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl tensorflow::SanitizeThreadSuffix(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" (?SanitizeThreadSuffix@tensorflow@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V23@@Z) [C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj]
08:30:35      4>C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\Release\pywrap_tensorflow_internal.dll : fatal error LNK1120: 1 unresolved externals [C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj]
08:30:35      4>Done Building Project "C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj" (default targets) -- FAILED.
08:30:35      1>Done Building Project "c:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\tf_python_build_pip_package.vcxproj" (default targets) -- FAILED.
08:30:35

ebrevdo · 2017-08-07T19:39:12Z

This is a python only change, I think?

…

On Aug 7, 2017 8:10 AM, "drpngx" ***@***.***> wrote: The Linux build is transient, but there is a windows build error: 08:30:35 4>measuring_cost_estimator.obj : error LNK2019: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl tensorflow::SanitizeThreadSuffix(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" ***@***.***@@***@***.******@***.***@std@@***@***.***@2@@std@@v23@@z) referenced in function "public: __cdecl tensorflow::grappler::MeasuringCostEstimator::MeasuringCostEstimator(class tensorflow::grappler::Cluster *,int,int)" ***@***.***@tensorflow@@***@***.***@***@***.***@z) [C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj] 08:30:35 4>queue_runner.obj : error LNK2001: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl tensorflow::SanitizeThreadSuffix(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" ***@***.***@@***@***.******@***.***@std@@***@***.***@2@@std@@v23@@z) [C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj] 08:30:35 4>single_machine.obj : error LNK2001: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl tensorflow::SanitizeThreadSuffix(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" ***@***.***@@***@***.******@***.***@std@@***@***.***@2@@std@@v23@@z) [C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj] 08:30:35 4>C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\Release\pywrap_tensorflow_internal.dll : fatal error LNK1120: 1 unresolved externals [C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj] 08:30:35 4>Done Building Project "C:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\pywrap_tensorflow_internal.vcxproj" (default targets) -- FAILED. 08:30:35 1>Done Building Project "c:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build\tf_python_build_pip_package.vcxproj" (default targets) -- FAILED. 08:30:35 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABtimyfi2aA2Y03b1ch_ZqYtAI-P5Yokks5sV1L6gaJpZM4MwTz9> .

drpngx · 2017-08-07T20:18:47Z

Right. Merging.

…assification * commit '6e054dbd4b741d5b8fa8af93fdd7c9b74ae67ce0': (511 commits) Fix tensordot with list of ints as axes (tensorflow#11959) Fix segfault when recording raw allocation returns nullptr (tensorflow#12074) [OpenCL] Fix for //tensorflow/python/kernel_tests:image_ops_test (tensorflow#111) (tensorflow#12041) Removing visited_node hash table - fixing multinode shape mismatch issue (tensorflow#12044) [OpenCL] Fixes core_rnn_cell_tests (tensorflow#12076) Added Convolutional LSTM (tensorflow#8891) Update monitors_test.py (tensorflow#12062) fix a typo in tf.nn.separable_conv2d's doc (tensorflow#12067) Fix cmake builds: (tensorflow#12048) Fix typo in RELEASE.md (tensorflow#12064) Fix typo (tensorflow#12069) Handle case where init node is not present in the frozen graph. Fix typo in datasets docstring tfdbg: fix a bug in string representation of SparseTensors BUILD dependency cleanup in tensorflow/stream_executor/cuda BUILD dependency cleanups. Rename RecvTensorAsync method to GrpcRecvTensorAsync to fix shadowing of method in Worker with a different signature. [tpu:profiler] Dump gzipped json trace. Minor cleanup tf.nn.separable_conv2d now supports data_format. Avoid unnecessary transposes in tf.layers.separable_conv2d (and implicitly in tf.contrib.layers.separable_conv2d). Add an identity initializer that works with partitioned variables. ...

lcnature · 2017-08-16T03:16:08Z

Hi @loliverhennigh I am trying to use it together with dynamic_rnn but got some errors. May I understand the input_shape better? Say I want the input to be video of size 32x32, 3 channels for RGB. Each video clip has 10 frames. What exactly should I feed to the input_shape argument? And let's say the batch_size is 5, how should the size of the inputs be if it is used together with dynamic_rnn?
Thank you！

loliverhennigh · 2017-08-24T22:28:45Z

Hey @Icnature, I wrote a little silly example of how to use the conv2dlstmcell with the dynamic_rnn stuff here
https://github.com/loliverhennigh/dynamic_rnn_conv_lstm/blob/master/mnist_deep.py#L57. To answer your question though, your input shape should be [32,32,3]. You can have your inputs be any shape that is supported by the dynamic rnn thing. In the example above the shape is [batch_size, seq_length, height, width, channels]. If you change time_major to True I think it will be [seq_length, batch_size, height, width, channels]. Hopefully that answers your question ok!

lcnature · 2017-08-24T23:17:33Z

Thanks @loliverhennigh ! I figured out my mistake. I was actually trying Conv1DLSTMcell. The kernel_shape should be a list instead of an integer.

rayanelleuch · 2017-10-17T06:37:22Z

@loliverhennigh
I feel like it is little bit disturbing to not be able to choose the padding style, stride, activation function, bias, etc like tf.contrib.layers.conv2d .
Is there a specific reason for that?

Linusnie · 2017-11-02T14:37:36Z

In the original paper they use peephole connections (i.e. the gates depend on the hidden state) but as far as I can tell these connections are not present in the implementation. Is this intentional? Might be worth it do mention in the documentation in that case.

carlthome · 2017-11-05T15:17:32Z

@Linusnie for now you can use my implementation here instead. I find peepholes important, by the way.

anjany · 2017-11-06T10:46:22Z

Hi @loliverhennigh!
Will I be able to use this for inputs with dynamic image shapes (consequently the cell shape would be changing too)?

I get a typical TypeError: int() argument must be a string or a number, not 'Tensor' for the code below. Am I doing something exceptionally wrong?

def _convlstm_layer(x):
	# x is shaped [batch_size, seq_len, _, _, 1]
	shape = [tf.shape(x)[2], tf.shape(x)[3], tf.shape(x)[4]]
        cell = tf.contrib.rnn.Conv2DLSTMCell(input_shape=shape,kernel_shape=[3,3], output_channels=8)
	(outputs, state) = tf.nn.dynamic_rnn(cell, x, time_major=False, dtype=tf.float32)
	return outputs

AllInNVDA · 2017-12-13T14:51:59Z

tensorflow/contrib/rnn/python/ops/rnn_cell.py

+  for shape in shapes:
+    if len(shape) not in [3,4,5]:
+      raise ValueError("Conv Linear expects 3D, 4D or 5D arguments: %s" % str(shapes))
+    if len(shape) != len(shapes[0]):


You could just use shape_length instead of recalculating it again.

Added Convolutional LSTM

3e99f74

googlebot added the cla: yes label Apr 1, 2017

loliverhennigh mentioned this pull request Apr 1, 2017

Convolutional RNN/LSTM #4536

Closed

yifeif requested a review from ebrevdo April 3, 2017 04:18

yifeif added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Apr 3, 2017

carlthome reviewed Apr 11, 2017

View reviewed changes

drpngx added stat:awaiting response Status - Awaiting response from author and removed stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Apr 14, 2017

loliverhennigh added 4 commits April 28, 2017 13:32

merged with tensorflow updates

a91a56a

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…

1f46cd3

…o feature/conv_lstm_rnn

fixed problems

3a44751

XMerge branch 'master' of https://github.com/tensorflow/tensorflow in…

cbaa5af

…to feature/conv_lstm_rnn

ebrevdo reviewed Jul 10, 2017

View reviewed changes

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…

c4b5507

…o feature/conv_lstm_rnn

vrv added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Jul 24, 2017

loliverhennigh added 6 commits July 24, 2017 16:54

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…

d4a530f

…o feature/conv_lstm_rnn

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…

c6d0956

…o feature/conv_lstm_rnn

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…

e5a0e5e

…o feature/conv_lstm_rnn

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…

cf4b56b

…o feature/conv_lstm_rnn

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…

89884d3

…o feature/conv_lstm_rnn

added variable size batch and img size

70556e6

lukaszkaiser approved these changes Aug 5, 2017

View reviewed changes

fix indent level

a884a92

drpngx merged commit e9682dd into tensorflow:master Aug 7, 2017

AllInNVDA reviewed Dec 13, 2017

View reviewed changes

Added Convolutional LSTM #8891

Added Convolutional LSTM #8891

Conversation

loliverhennigh commented Apr 1, 2017

tensorflow-jenkins commented Apr 1, 2017

marcociccone commented Apr 3, 2017 • edited

ebrevdo commented Apr 3, 2017 via email

marcociccone commented Apr 3, 2017

ebrevdo commented Apr 3, 2017 via email

marcociccone commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhack commented Apr 24, 2017

carlthome commented Apr 24, 2017

bhack commented Apr 28, 2017

loliverhennigh commented May 1, 2017

vrv commented May 1, 2017

ebrevdo commented May 2, 2017

vrv commented May 4, 2017

loliverhennigh commented May 5, 2017

ebrevdo commented May 5, 2017 via email

ebrevdo left a comment

Choose a reason for hiding this comment

vrv commented Jul 24, 2017

loliverhennigh commented Aug 5, 2017

lukaszkaiser left a comment

Choose a reason for hiding this comment

drpngx commented Aug 5, 2017 via email

drpngx commented Aug 7, 2017

drpngx commented Aug 7, 2017

ebrevdo commented Aug 7, 2017 via email

drpngx commented Aug 7, 2017

lcnature commented Aug 16, 2017 • edited

loliverhennigh commented Aug 24, 2017

lcnature commented Aug 24, 2017

rayanelleuch commented Oct 17, 2017 • edited

Linusnie commented Nov 2, 2017

carlthome commented Nov 5, 2017

anjany commented Nov 6, 2017

Choose a reason for hiding this comment

marcociccone commented Apr 3, 2017 •

edited

lcnature commented Aug 16, 2017 •

edited

rayanelleuch commented Oct 17, 2017 •

edited