Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @WenheLI . The biggest comment I have right now is: please add unit tests by creating a new file called src/layers/noise_test.ts.
Also see some small comments below.
Please address the comments and I'll review the PR again.
Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)
src/layers/noise.ts, line 25 at r1 (raw file):
/**
style: Doc string should go above the class, not below it. Follow the pattern in the other examples. Same elsewhere.
src/layers/noise.ts, line 47 at r1 (raw file):
style nit: No multiple empty lines. Only one is fine. The linter should have caught that. Same elsewhere.
Thanks @caisq . I will work on the unit test soon. Just one thing I notice that namely most layers are exported by |
@WenheLI Good question! I forgot to comment on that. Please follow the patterns of other layers (e.g., Dense in core.ts) in order to export the factory method of the new layer type under the tf.layers.* namespace. |
Hi, I just added some symbolic tests on the noise layers. Do I need to add more tests to this unit? Since I found it might be hard to test these layers with actual values, I might need some references on it to go further? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)
src/layers/noise.ts, line 25 at r3 (raw file):
Standard Deviation
Style nit: all comments should end in a period. Same elsewhere.
src/layers/noise.ts, line 30 at r3 (raw file):
Gaussian noise.
It's help to add a comment here that the additive Gaussian Noise is applied only during
training. I know the Python doc string doesn't mention that. So it would be good to
fix that as well (in keras and tensorflow repos, as separate PRs, that is).
src/layers/noise.ts, line 77 at r3 (raw file):
(
Remove superfluous pair of parentheses.
src/layers/noise.ts, line 77 at r3 (raw file):
(
Style: Line continuation indent is 4 chars. Same elsewhere.
src/layers/noise.ts, line 93 at r3 (raw file):
* Apply multiplicative 1-centered Gaussian noise. *
Like I commented above, it's helpful to mention that the multiplicative noise is
applied only during training.
src/layers/noise.ts, line 141 at r3 (raw file):
1.0
nit: JavaScript / TypeScript don't distinguish float and int. All numbers are float. So remove the decimal points and the trailing zero. Same elsewhere.
src/layers/noise.ts, line 169 at r3 (raw file):
*
nit: Remove extra line.
src/layers/noise.ts, line 223 at r3 (raw file):
rate = this.rate
Why include this additional argument? The value of this.rate is always used anyway.
src/layers/noise.ts, line 223 at r3 (raw file):
input = inputs,
No need to have a default value here.
src/layers/noise_test.ts, line 12 at r3 (raw file):
* Unit Tests for Noise Layers
Apart from these symbolic tests, we need tests based on concrete tensor values.
Follow the examples for other layers such as Dense.
@caisq Thanks for your comments. I have fixed most of your comments. Just one more thing remains to be unfinished that is the unit tests for concrete values. Since all the three noise layers will get random results, I am thinking test if their outputs will be a normal distribution that requires a function Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WenheLI Good question about how to test the new layers with concrete values. As you pointed out, the outputs will be random (during training, that is). I suggest cover the following aspects:
- That the
call()
method executes without any error when the kwargtraining
is set tofalse
, and that the output
tensor should be equal to the input tensor. - That the
call()
method executes without any error when the kwargtraining
is set totrue
, and that the output has the same dtype and shape as the input. In addition, the output should be unequal to the input. Note that we currently don't have a method likeexpectTensorsUnequal
. But you can do that kind of assertion by calculating the maximum of the absolute difference between the input and output, e.g.,output.sub(input).abs().max()
and assert the result is greater than 0. - That two consecutive calls to
call()
give different results, if kwargtraining
istrue
.
Thanks.
Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)
@caisq Thanks for your suggestions! I have added the unit test. Let me know if there is any further problem! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, @WenheLI . Let's wait for the tests to pass. Then we can merge the PR.
Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)
This PR is about tensorflow/tfjs#1326
I added three noise layers described at Keras documentation
https://keras.io/layers/noise/
Thanks
This change is