Skip to content
This repository has been archived by the owner on Oct 17, 2021. It is now read-only.

Add getConfig() to layers Concatenate and Bidirectoinal #206

Merged
merged 2 commits into from
May 23, 2018

Conversation

caisq
Copy link
Contributor

@caisq caisq commented May 22, 2018

with unit and integration tests.

Fixes: tensorflow/tfjs#328

BUG: Fixes the saving and loading of Concatenate layers (tf.layers.concatenate).


This change is Reviewable

@bileschi
Copy link
Contributor

Review status: 0 of 5 files reviewed at latest revision, all discussions resolved.


integration_tests/tfjs2keras/tfjs_save.ts, line 111 at r1 (raw file):

Functioal

Can you fix this spelling error while you're in there? Thansk ;)


src/layers/wrappers.ts, line 475 at r1 (raw file):

// TODO(cais): Add logic for `numConstants` once the property is added.

Would it be safer to throw NotImplementedError if the object has a numConstants?


src/layers/wrappers.ts, line 486 at r1 (raw file):

    // TODO(cais): Add logic for `numConstants` once the property is added.

Probably no need for the TODO, given the NotImplementedError


Comments from Reviewable

@bileschi
Copy link
Contributor

:lgtm_strong:


Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@caisq
Copy link
Contributor Author

caisq commented May 23, 2018

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.


integration_tests/tfjs2keras/tfjs_save.ts, line 111 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
Functioal

Can you fix this spelling error while you're in there? Thansk ;)

Done.


src/layers/wrappers.ts, line 475 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
// TODO(cais): Add logic for `numConstants` once the property is added.

Would it be safer to throw NotImplementedError if the object has a numConstants?

this.numConstants is currently undefined.


src/layers/wrappers.ts, line 486 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
    // TODO(cais): Add logic for `numConstants` once the property is added.

Probably no need for the TODO, given the NotImplementedError

Let's keep this TODO item. It's better to be redundant than to miss something.


Comments from Reviewable

@caisq caisq merged commit c1663a8 into tensorflow:master May 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants