Support for ConvLSTM2D

[dikatok]

Support for ConvLSTM2D.

Fixes #3637.

---

This change is 

[dikatok]

@dsmilkov @pyu10055

I modified this check since the default recurrent initializer in ConvLSTM2DCell is orthogonal which needs to support 4D.

This update seems to be inline too with Python implementation (link below).

https://github.com/tensorflow/tensorflow/blob/6016e8009ff8d9b3afa5ae5bafd052ed51d22f74/tensorflow/python/ops/init_ops.py#L571

[dikatok]

Known incompatible things:

• batchNormalization if returnSequences: true Error: batchNormalization is not implemented for array of rank 5 yet
• running ConvLSTM2DCell via RNN or StackedRNN, although this should be discouraged
• goBackwards: true Error: WebGL backend: Reverse of rank-5 tensor is not yet supported
• gather (webgl) Error: Gather for rank 5 is not yet supported

[dikatok]

@caisq

class ConvRNN2D extends RNN {

How verbose would this docstring need to be?

if (this.cell.dropoutMask != null) {
        tfc.dispose(this.cell.dropoutMask);
        this.cell.dropoutMask = null;
      }

Actually, I was following the way it is structured in recurrent cells, not quite sure about the why either since I could not find any from the commit changes.

generateDropoutMask

Should I update the one in recurrent.ts with mine? I think it looks cleaner.

[caisq]

How verbose would this docstring need to be?

What you added seems fine to me.

Actually, I was following the way it is structured in recurrent cells, not quite sure about the why either since I could not find any from the commit changes.

I understand it now.

Should I update the one in recurrent.ts with mine? I think it looks cleaner.

Yes please. Make sure they are functionally equivalent (they are covered by tests, so as long as tests all pass we are good).

Reviewable status: 0 of 1 approvals obtained (waiting on @dikatok, @dsmilkov, and @pyu10055)

---

tfjs-layers/src/layers/convolutional_recurrent.ts, line 127 at r6 (raw file):

Previously, caisq (Shanqing Cai) wrote…

if (this.cell.dropoutMask != null) {
        tfc.dispose(this.cell.dropoutMask);
        this.cell.dropoutMask = null;
      }

I'm not sure I understand the logic here. Why discard dropoutMaskif it is set?

I think I understand now. The previous dropout mask should be disposed to make way for new ones.

---

tfjs-layers/src/layers/convolutional_recurrent_test.ts, line 520 at r9 (raw file):

'padding': padding,

Please fix the indentation in the Python code here.

Same below.

---

tfjs-layers/src/layers/convolutional_recurrent_test.ts, line 536 at r9 (raw file):

Quoted 4 lines of code…

s_1 = np.ones([batch_size, sequence_len, data_size, data_size,
   * data_channel]) xs_2 = np.zeros([batch_size, sequence_len, data_size,
   * data_size, data_channel]) xs = np.concatenate([xs_1, xs_2], 0) ys =
   * np.array([[1], [1], [1], [1], [0], [0], [0], [0]])

The new line characters are incorrectly discarded, it appears. Can you fix it?

Same for the Python code in the comments below.

[dikatok]

@caisq Done!

Thanks for the quick feedback loop, by the way, I thought the PR would be dangling for quite a while.

Also, would you mind taking a look at my previous comments up there and see if you have any suggestions?

[caisq]

@dsmilkov @pyu10055 let me know if you have any further comments. If not, I'll be fine with merging the PR as soon as the tests pass.

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dikatok, @dsmilkov, and @pyu10055)

[caisq]

Looks like there are lint errors.

ERROR: (no-unnecessary-type-assertion) /workspace/tfjs-layers/src/layers/convolutional_recurrent.ts[160, 35]: This assertion is unnecessary since it does not change the type of the expression.

@dikatok Can you run yarn lint locally and fix all the errors?

[pyu10055]

Reviewed 1 of 3 files at r3, 1 of 3 files at r5, 2 of 6 files at r6, 2 of 3 files at r10, 1 of 1 files at r11.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @dikatok, @dsmilkov, and @pyu10055)

---

tfjs-layers/src/exports_layers.ts, line 1356 at r11 (raw file):

}

export function convLstm2d(args: ConvLSTM2DArgs): ConvLSTM2D {

are jsdoc missing for these two methods?

[caisq]

@dikatok The code snippet in the comments should use tf.* instead of tfc.*. This is why the CI is failing.

const filters = 3;
const kernelSize = 3;

const sequenceLength = 1;
const size = 5;
const channels = 3;

const inputShape = [sequenceLength, size, size, channels];
const input = tfc.ones(inputShape);

const cell = tf.layers.convLstm2dCell({filters, kernelSize});

cell.build(input.shape);

const outputSize = size - kernelSize + 1;
const outShape = [sequenceLength, outputSize, outputSize, filters];

const initialH = tfc.zeros(outShape);
const initialC = tfc.zeros(outShape);

const [o, h, c] = cell.call([x, initialH, initialC], {});

[dikatok]

@caisq ah okay, will fix it later

[dikatok]

@caisq can this jsdoc assertion performed locally? or it is only run in CI?

[caisq]

@dikatok The snippet-testing code is here:
https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/scripts/test_snippets.ts

I'm not sure about the correct command to invoke it. But you can try figuring it out or maybe @pyu10055 can tell you more. It runs on CI for sure.

[caisq]

Actually, running yarn test-snippet in the tfjs-layers folder should be the way. See:
https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/package.json#L65

[dikatok]

Ah my bad, it should be fixed now.

About these issues I encountered earlier, should they be handled in separate issues/PR in the future?

• batchNormalization if returnSequences: true Error: batchNormalization is not implemented for array of rank 5 yet
• running ConvLSTM2DCell via RNN or StackedRNN, although this should be discouraged
• goBackwards: true Error: WebGL backend: Reverse of rank-5 tensor is not yet supported
• gather (webgl) Error: Gather for rank 5 is not yet supported

[caisq]

@pyu10055 PTAL? I think PR is ready to merge.

[dikatok]

@caisq BTW, is it acceptable for outside contributors like me to do code refactoring? I kind of want to refactor the recurrent.ts file to make it easier to digest. Probably will start with arguments inheritance and the unnecessary getters.

[caisq]

@dikatok I think it is acceptable. I'd suggest doing it as a separate PR from this one, though.