[Refactoring] Clean up recurrent cells and layers getConfig

[dikatok]

The purpose of this PR is to clean up the getConfig method related to recurrent and convolutional_recurrent layers and cells for better readability.

---

This change is 

[dikatok]

Regarding recurrent_test.ts, the actual changes are in the following lines:

tfjs/tfjs-layers/src/layers/recurrent_test.ts

Line 1549 in 315b68b

expect(layerPrime.getConfig()['recurrentActivation'])

tfjs/tfjs-layers/src/layers/recurrent_test.ts

Line 1564 in 315b68b

expect(layerPrime.getConfig()['recurrentActivation'])

Not quite sure why a lot of formatting occurred.

[pyu10055]

Thank you for refactoring, on high level can you explain your approach to eliminate the manual hierarchical construction of the config?

Reviewable status: 0 of 1 approvals obtained

[dikatok]

Thank you for refactoring, on high level can you explain your approach to eliminate the manual hierarchical construction of the config?

Reviewable status: 0 of 1 approvals obtained

@pyu10055 Sure, here are my observations.

1. Layer getConfig is simply re-exposing the respective cell's getConfig + specific recurrent configs (returnSequences, returnState, stateful, etc). We can achieve the same by merging the underlying cell config and layer config, instead of repeating them.
   Cautionary needs to be taken during the merge though, to avoid core config such as layerName to be replaced.

2. Since now we can delegate export of cell configs to themselves, we can also remove getConfig overrides from each RNN layers (LSTM, GRU, Stacked, ConvLSTM) and let the superclass which in this case RNN to handle it.

[pyu10055]

Thank you for explanation, overall your change looks good to me.
Can you add tests that verify the config generated in the new way matches to the config generated by original code? thanks

Reviewable status: 0 of 1 approvals obtained (waiting on @dikatok)

---

tfjs-layers/src/layers/recurrent_test.ts, line 1564 at r1 (raw file):

    const yPrime = layer.apply(x) as Tensor;
    expectTensorsClose(yPrime, y);
    expect(layerPrime.getConfig()['recurrentActivation'])

can you test against a constant config, comparing two same type of layers does not seem to guarantee no change to the exist behavior.

[dikatok]

Thank you for explanation, overall your change looks good to me.
Can you add tests that verify the config generated in the new way matches to the config generated by original code? thanks

Reviewable status: 0 of 1 approvals obtained (waiting on @dikatok)

tfjs-layers/src/layers/recurrent_test.ts, line 1564 at r1 (raw file):

    const yPrime = layer.apply(x) as Tensor;
    expectTensorsClose(yPrime, y);
    expect(layerPrime.getConfig()['recurrentActivation'])

can you test against a constant config, comparing two same type of layers does not seem to guarantee no change to the exist behavior.

Added the requested tests, all values were generated from the previous version of the code. I think the new tests resolve your last comment as well?

[dikatok]

@pyu10055 friendly ping 🦝

[pyu10055]

Thanks you for the refactoring!

Reviewed 3 of 3 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @pyu10055)

[pyu10055]

Reviewable status: 0 of 1 approvals obtained (waiting on @dikatok)

---

tfjs-layers/src/layers/recurrent_test.ts, line 1564 at r1 (raw file):

Previously, dikatok (Andika Tanuwijaya) wrote…

Added the requested tests, all values were generated from the previous version of the code. I think the new tests resolve your last comment as well?

thank you for adding the detail tests.

[pyu10055]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dikatok)