update docs for lrn by nswamy · Pull Request #5913 · apache/mxnet

nswamy · 2017-04-20T11:36:19Z

@mli, @zackchase, @madjam, @Roshrini , @indhub @jiajiechen

Please review

zackchase

Thanks @nswamy for the contributions. Please see the few minor comments and let me know if they are sensible or if you disagree.

zackchase · 2017-04-20T17:29:32Z

src/operator/lrn-inl.h

  DMLC_DECLARE_PARAMETER(LRNParam) {
    DMLC_DECLARE_FIELD(alpha).set_default(1e-4f)
-    .describe("value of the alpha variance scaling parameter in the normalization formula");
+    .describe("Alpha variance scaling parameter in the normalization formula.");


Here "alpha is used as an adjective. I think it might be more intuitive to write:

"The variance scaling parameter alpha from the normalization formula."

Where is the normalization formula? How does the user find what alpha refers to? Should we have some guidance for where this reference "normalization formula" points to? What are your thoughts??

Hey any luck resolving this one? Thanks!

LRN expression is in the description of the function

zackchase · 2017-04-20T17:29:52Z

src/operator/lrn-inl.h

+    .describe("Alpha variance scaling parameter in the normalization formula.");
    DMLC_DECLARE_FIELD(beta).set_default(0.75f)
-    .describe("value of the beta power parameter in the normalization formula");
+    .describe("Beta power parameter in the normalization formula.");


Same issue - let's make it consistent with however we do alpha.

zackchase · 2017-04-20T17:30:07Z

src/operator/lrn-inl.h

+    .describe("Beta power parameter in the normalization formula.");
    DMLC_DECLARE_FIELD(knorm).set_default(2.0f)
-    .describe("value of the k parameter in normalization formula");
+    .describe("k parameter in normalization formula.");


zackchase · 2017-04-20T17:30:54Z

src/operator/lrn.cc

+.add_argument("data", "NDArray-or-Symbol", "Input data.")
 .add_arguments(LRNParam::__FIELDS__())
-.describe("Apply convolution to input then add a bias.");
+.describe(R"code(Applies Local Response Normalization to the input.


Let's not have this in capital letters. Just

"Applies local response normalization to the input."

The main reasons to use capital letters would be if we are introducing a proper noun or an acronym.

zackchase · 2017-04-20T17:34:42Z

src/operator/lrn.cc

-.describe("Apply convolution to input then add a bias.");
+.describe(R"code(Applies Local Response Normalization to the input.
+
+Normalization helps in generalization and used to prevent neurons from 


Maybe be more descriptive here. Not all neurons can saturate. Also there is a grammatical bug on "used to" (suggests in the past, not utility).

Perhaps: "Normalization helps to prevent squashing neurons (like tanh) from saturating. It can also help for transfer learning, as the inputs for each task will exhibit the same dynamic range."

LRN is applied on a ReLU neuron. Are ReLU neurons also considered squashing neurons ?

what I was aiming for was to write a sentence about Normalization and then describe LRN. I will remove it if this leads to confusion.
You are right that ReLU neurons don't saturate and might turn into on negative inputs.

zackchase · 2017-04-20T17:58:33Z

No but they don't saturate! I'm referring specifically to the comment about normalization. Perhaps it was copied from the doc for a different activation function that actually does saturate?

…

On Thu, Apr 20, 2017 at 1:49 PM, Naveen Swamy ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/operator/lrn.cc <#5913 (comment)>: > .add_arguments(LRNParam::__FIELDS__()) -.describe("Apply convolution to input then add a bias."); +.describe(R"code(Applies Local Response Normalization to the input. + +Normalization helps in generalization and used to prevent neurons from LRN is applied on a ReLU neuron. Are ReLU neurons also considered squashing neurons ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5913 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACR4zrbe7IwspDGuuM0ht0jeT7bLBgvyks5rx5qYgaJpZM4NC5_0> .

* InstanceNorm doc modified * minor change

nswamy

Thanks @zackchase, I have made changes per your suggestions.

nswamy · 2017-04-27T20:26:41Z

src/operator/lrn-inl.h

  DMLC_DECLARE_PARAMETER(LRNParam) {
    DMLC_DECLARE_FIELD(alpha).set_default(1e-4f)
-    .describe("value of the alpha variance scaling parameter in the normalization formula");
+    .describe("Alpha variance scaling parameter in the normalization formula.");


LRN expression is in the description of the function

* update doc for lrn * [Documentation] Ndarray instancenorm doc modified (apache#6008) * InstanceNorm doc modified * minor change * xx

zackchase reviewed Apr 20, 2017

View reviewed changes

madjam added the Doc label Apr 20, 2017

Naveen Swamy and others added 3 commits April 27, 2017 13:47

update doc for lrn

68f10d0

[Documentation] Ndarray instancenorm doc modified (#6008)

d14f4b8

* InstanceNorm doc modified * minor change

xx

d426c2c

nswamy commented Apr 27, 2017

View reviewed changes

madjam approved these changes Apr 27, 2017

View reviewed changes

piiswrong merged commit b5903c3 into apache:master Apr 27, 2017

nswamy deleted the lrn branch April 27, 2017 22:11

Guneet-Dhillon pushed a commit to Guneet-Dhillon/mxnet that referenced this pull request Sep 13, 2017

update docs for lrn (apache#5913)

b0cee89

* update doc for lrn * [Documentation] Ndarray instancenorm doc modified (apache#6008) * InstanceNorm doc modified * minor change * xx

Conversation

nswamy commented Apr 20, 2017

Uh oh!

zackchase left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zackchase commented Apr 20, 2017 via email

Uh oh!

nswamy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants