Adding TCN Layers + Tutorial to TFA Layers

[shun-lin]

This PR adds TCN layer as well as the Residual Block layer to TFA layers. A colab with the basic usage of TCN layer with IMDB is also included.

TODO(shunlin):
Write more tests.
May I get some pointers on what kind of tests I should include? Thanks!

Associated Issues:
#661
philipperemy/keras-tcn#73

Source Implementations:
https://github.com/philipperemy/keras-tcn (In Pure Keras)
https://github.com/locuslab/TCN (In Pytorch)

Relevant Paper:
https://arxiv.org/pdf/1803.01271.pdf

[review-notebook-app]

Check out this pull request on 

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

[WindQAQ]

Thanks for the contribution! My main concern is why we should instantiate layers in build instead of __init__. Could you elaborate the reason why?

[shun-lin]

Thanks for the contribution! My main concern is why we should instantiate layers in build instead of __init__. Could you elaborate the reason why?

Hi Tzu-Wei @WindQAQ, thank you so much for the review, you are right that there are some layers that are not depend on the input_shape should be instantiated in __init___, I will update the code to reflect that, thank you so much!

[shun-lin]

@WindQAQ I have resolved the comments you had earlier this week and updated the code, when you have time may you review the changes? Thank you so much!

[Squadrick]

@shun-lin The code looks good so far. A few changes requested.

Could you also expand the tests?

1. We need tests for both TCN and ResidualBlock.
2. Check that they can be serialized and deserialized. Both the layers are currently missing from_config.

I haven't taken a look at the notebook yet, but I'll do that soon.

[shun-lin]

@shun-lin The code looks good so far. A few changes requested.

Could you also expand the tests?

1. We need tests for both TCN and ResidualBlock.
2. Check that they can be serialized and deserialized. Both the layers are currently missing from_config.

I haven't taken a look at the notebook yet, but I'll do that soon.

got it, thanks @Squadrick !

[shun-lin]

@shun-lin The code looks good so far. A few changes requested.

Could you also expand the tests?

1. We need tests for both TCN and ResidualBlock.
2. Check that they can be serialized and deserialized. Both the layers are currently missing from_config.

I haven't taken a look at the notebook yet, but I'll do that soon.

@Squadrick I believe that we don't need to extend from_config method from tf.keras.layer's from_config as the from_config method that is inherited should work just fine, but will definitely write unit tests to confirm this :)

Edit:
Tests for serialization and deserialization for both TCN and ResidualBlock are included.

Edit 2:
I ran into a potential bug in tensorflow's layer_test when writing tests for ResidualBlock as the layer_test will break when the layer returns a list/tuple of tensors, I have submitted an issue on tensorflow's github and here is the link:
tensorflow/tensorflow#34408

[Squadrick]

@shun-lin Thanks for making the changes. I think you've accidentally included an extra file: env/bin/tensorboard.

I'm adding a BLOCKED tag to this PR until the issue with layer_test is resolved.

[shun-lin]

@shun-lin Thanks for making the changes. I think you've accidentally included an extra file: env/bin/tensorboard.

I'm adding a BLOCKED tag to this PR until the issue with layer_test is resolved.

@Squadrick thanks for catching the extra file, I have removed env/bin/tensorboard in the latest patch. I will try to write more tests for TCN and wait on the issue with layer_test for test for residual block.

[shun-lin]

@Squadrick may you unblock this PR? I think the issue with layer_test should not be a blocking issue for this PR, I'll write more test for TCN layer and a TODO for adding tests for the internal residual block once the issue with layer_test is resolved. Thanks!

[Squadrick]

@shun-lin I'd prefer if we don't merge the PR until the tests are up and working. Three reasons for this:

1. Maintaining it will be a hassle since we can't check if any changes made to ResidualBlock are broken.
2. Any upstream changes in TF that cause ResidualBlock to change its behaviour won't be caught immediately.
3. We have no ETA on when the blocking issue will be resolved.

Will test_tcn catch any errors caused in ResidualBlock? If yes, then we can reconsider merging the PR with a TODO.

[shun-lin]

@Squadrick you have a good point, and the tensorflow team has assigned the blocking issue to me so I will look into resolving it when I have time. In the meantime I will write more tests for tcn and see if I can catch errors caused in ResidualBlock as you suggested. Thanks for the feedback!

[shun-lin]

@Squadrick so after digging inside Tensorflow Keras's codebase for tests for layers that are returning a list of Tensors, I found out that they are not using layer_test, but instead writing custom tests (see below for reference), and that layer_test does specifically said that it only works for "single-input single-output" layers and therefore at layer_test's current state I can not use it to test ResidualBlock, I will write custom tests for ResidualBlock without the use of layer_test and update it to layer_test when it finally support layers that have list of Tensors as output. Thanks!

tf.keras.layers.RNN's test when the layer is set (with 'return_sequences' set to 'True) to return a list of Tensors:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/keras/layers/recurrent_test.py#L330

[Squadrick]

@shun-lin Thanks for looking into this. Yes, that sounds like a good idea.

update it to layer_test when it finally support layers that have list of Tensors as output.

Make this a TODO in the code.

[pedrolarben]

Thanks for this contribution @shun-lin. I am really interested in using TCN layers. If you need any help, let me know. I will be happy to help.

[philipperemy]

@shun-lin any update so far on this one? :)

[seanpmorgan]

Seems like the path forward here is already determined, but just wanted to mention that it was previously suggested we fork layer_test and expose it as a public API for other projects to use:
#258

[shun-lin]

@pedrolarben @philipperemy @seanpmorgan

I apologize for the delay, the main issue I have right now is just testing the internal ResidualBlock layers, which returns a list of tensors, and since tf.keras's layer_test does not currently support testing layers that returns a list of tensors, I am working on extending layer_test to accomplish this, but it is taking longer than I expected. I'll try to put more time into this and contribute to update layer_test during the upcoming holiday breaks :)

Relevant Tensorflow Issue:
tensorflow/tensorflow#34408

And @pedrolarben I think @philipperemy's Keras-TCN works with TF2 now as well, https://github.com/philipperemy/keras-tcn.

And @philipperemy if you have written any tests for TCN please let me know so I don't duplicate work, thanks!

[philipperemy]

@shun-lin I can confirm that it now works well with TF2. There are no unit tests at the moment for TCN. Just a quick training test to make sure that the training converges after a couple of epochs on a simple task.

[gabrieldemarmiesse]

This pull request had no activity for a while now. I'll be closing it. Feel free to open it again if there are changes.