[fix] missing input log higher order. #15331

kshitij12345 · 2019-06-23T05:38:27Z

@larroy Thank You very much for catching this.

Sorry for the silly mistake.
Can we have a way to test this?

roywei · 2019-07-08T16:21:17Z

@mxnet-label-bot add [Operator, Backend]

larroy · 2019-07-09T19:36:36Z

I'm still not sure what's the meaning of the backward output for the head gradient input as we discussed before. This week we are at a conference so we might be slow to respond.

I'm not sure how to test this, I think I would need to dump the graph and think about it, as I'm not sure now in which python variable is the gradient of the head gradient stored.

I think the PR fixes the issue though. Would the operator had failed on a division without argument? looks like the tests don't execute the Op or?

Would it be better to set those outputs to zero since we don't know how to use them? I'm fine with the fix proposed in this PR though.

apeforest · 2019-07-10T20:18:20Z

@larroy Those outputs are needed for 3rd order and above gradients.

apeforest · 2019-07-10T20:21:48Z

@kshitij12345 https://github.com/apache/incubator-mxnet/pull/15331/files#diff-0dad60704ce39e602a1907aec6835375R1121 comment should actually be dL/dygrad. Could you please update it as well?

apeforest · 2019-07-10T20:26:08Z

@sxjscience Do we have a use case where the gradient on the gradient of output y is needed?
i.e. ygrad = dL/dy. How can we test the value of dG/dygrad given G is a function G(x, y, xgrad, ygrad) from R^n -> R.

apeforest · 2019-07-10T20:26:52Z

Please update the comment, otherwise LGTM

kshitij12345 · 2019-07-11T04:43:55Z

https://github.com/apache/incubator-mxnet/blob/5171e1d92cfc5eefa2c20dfe8ac3fac5351ad19a/src/operator/tensor/elemwise_unary_op_basic.cc#L1120
dL/dygrad for this one right?

@larroy @apeforest , I was also wondering if we can check the number of inputs passed at compile time? I have observed the MakeNode gets the Op from dynamic registry based on the name. However we actually have information about the number of inputs and outputs for a given Op at compile time. I tried but couldn't actually figure out. What are your thoughts? How easy or hard would it be to check for valid number of inputs in MakeNode? This would help catch these sort of errors at compile time itself.

larroy · 2019-07-16T20:45:26Z

I think ograd[0] is dL/dx_grad

About the number of inputs, you are right that we could check. If it's more than one or two function calls I think is too much overhead and it's going to get caught with the python tests, Also if you don't return enough gradients, there's a check after calls to fgradient, so I think is not a big deal. Up to you if you can come up with something concise.

larroy · 2019-07-16T20:47:02Z

src/operator/tensor/elemwise_unary_op_basic.cc

@@ -1117,15 +1117,15 @@ MXNET_OPERATOR_REGISTER_BINARY_WITH_SPARSE_CPU_DR(_backward_log10,
                                                  unary_bwd<mshadow_op::log10_grad>)
 .set_attr<nnvm::FGradient>("FGradient",
  [](const nnvm::NodePtr& n, const std::vector<nnvm::NodeEntry>& ograds) {
-    // ograds[0]: dL/dxgrad
+    // ograds[0]: dL/dygrad


I think this is dL/dx_grad. The head gradient is the gradient with respect to the previous output right? the previous output is x_grad or dL/dx so this thing is dL/(dL/dx) or dL/dx_grad in lack of a better notation.

I guess it should be, dL/dy_grad as we are computing/returning dL/dx_grad,
Eg.

y = f(dx_grad) L = g(y) # dx_grad formed part of the network and affected loss During backprop by chain rule, dL/dx_grad = dL/dy * dy/dx_grad In comments, we have called dL/dy (mentioned in the above example) as dL/dy_grad

That is why we have,
https://github.com/apache/incubator-mxnet/blob/5b95fb3ee3581ba20fe1def336621d68a811e17f/src/operator/tensor/elemwise_unary_op_basic.cc#L1111-L1112

These multiplications performing,

dL/dx_grad = dL/dy * dy/dx_grad

I think the notation is complicating us in excess as it gets pretty hairy. It's the head gradient of the previous (output) node, which has shape of x, and x_grad. So it has to be related to x, not y.

I think in Lagrange notation it would be $$F_{L_x}$$ (derivative of some head function with respect to the derivative of the first loss wrt to x. (x_grad).

Oh. I get it now. If I understand it correctly then, crudely ograds[0] is how much does the x_grad affect the L and then we compute how does x_grad change with x. Makes sense now.

Thank you very much. Will reflect it in this and other PRs.

@kshitij12345 I think what you write makes sense. I'm also unsure about notations, maybe you can come with a better one. If not maybe we leave the comment out, so we can merge the PR, as the code seems to do what's needed.

Sure. Thanks Again.

larroy · 2019-07-16T20:54:21Z

src/operator/tensor/elemwise_unary_op_basic.cc

    // inputs[0]: dL/dy
-    // inputs[1]: x
+    // inputs[1]: x (ElemewiseGradUseIn)


nice comment, helps.

…to fix/missing-input

apeforest

LGTM

…to fix/missing-input

karan6181 · 2019-08-29T17:32:21Z

@kshitij12345 Could you please resolve the merge conflict and ping the reviewers again to merge it? Thanks!

kshitij12345 · 2019-08-30T16:04:18Z

Sure. Thanks

kshitij12345 · 2019-09-11T16:40:04Z

@larroy @apeforest Gentle ping for review.

kshitij12345 · 2019-09-19T17:03:32Z

@apeforest @larroy Gentle Ping.

sxjscience · 2019-09-19T18:12:42Z

I guess we need to add a test case.

kshitij12345 · 2019-09-20T16:42:58Z

@sxjscience I am not sure about how to test this. I was expecting that this missing input will cause problem with computing of higher order. Tried for 3rd order, which was computed successfully. For fourth order,
Operator _backward_mul is non-differentiable because it didn't register FGradient attribute (different problem). So I am not very sure as to how to write a test case.

kshitij12345 · 2019-10-20T08:06:08Z

@sxjscience @apeforest @larroy Gentle Ping.

larroy

Sorry we are all quite busy. I think it's fine to merge this. We can do any additional refinements later.

LGTM

apeforest

LGTM. Sorry for the delayed response. We have been extremely busy in the past month.

kshitij12345 · 2019-11-22T07:21:19Z

@apeforest Sure no worries. Thanks.

fix missing input

5171e1d

Roshrini added the pr-awaiting-review PR is waiting for code review label Jun 23, 2019

marcoabreu added Backend Issues related to the backend of MXNet Operator labels Jul 8, 2019

kshitij12345 changed the title ~~[fix] missing input log higher order~~ [fix] missing input log higher order. Jul 8, 2019

update comments

5b95fb3

kshitij12345 mentioned this pull request Jul 15, 2019

[MXNET-978] Higher Order Gradient Support arctan, arctanh, radians. #15531

Merged

7 tasks

larroy suggested changes Jul 16, 2019

View reviewed changes

larroy mentioned this pull request Jul 16, 2019

[MXNET-978] Higher Order Gradient Support arcsinh, arccosh. #15530

Merged

7 tasks

larroy reviewed Jul 16, 2019

View reviewed changes

kshitij12345 mentioned this pull request Jul 19, 2019

[MXNET-978] Higher Order Gradient Support sinh, cosh. #15412

Merged

7 tasks

kshitij12345 added 2 commits July 26, 2019 20:19

update comments

a64b35f

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

6c5354c

…to fix/missing-input

apeforest approved these changes Jul 30, 2019

View reviewed changes

larroy approved these changes Jul 30, 2019

View reviewed changes

kshitij12345 added 2 commits July 31, 2019 19:57

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

0581432

…to fix/missing-input

retrigger CI

bfd2d9b

kshitij12345 added 3 commits August 30, 2019 23:04

merge latest 'master' into fix/missing-input

7574c16

retrigger CI

fa51e35

Merge branch 'master' into fix/missing-input

7e354f0

kshitij12345 added 2 commits September 7, 2019 12:43

update node_op_util

9885e62

use NodeOpGen for _backward_log*

09ada96

kshitij12345 requested a review from apeforest September 7, 2019 07:17

kshitij12345 added 2 commits September 7, 2019 14:37

retrigger cI

15ef0ba

retrigger CI

60e51eb

larroy approved these changes Nov 1, 2019

View reviewed changes

apeforest approved these changes Nov 19, 2019

View reviewed changes

apeforest merged commit 60f53ed into apache:master Nov 19, 2019

kshitij12345 deleted the fix/missing-input branch November 22, 2019 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] missing input log higher order. #15331

[fix] missing input log higher order. #15331

kshitij12345 commented Jun 23, 2019

roywei commented Jul 8, 2019

larroy commented Jul 9, 2019 •

edited

Loading

apeforest commented Jul 10, 2019

apeforest commented Jul 10, 2019

apeforest commented Jul 10, 2019 •

edited

Loading

apeforest commented Jul 10, 2019

kshitij12345 commented Jul 11, 2019

larroy commented Jul 16, 2019

larroy Jul 16, 2019

kshitij12345 Jul 17, 2019

larroy Jul 25, 2019 •

edited

Loading

kshitij12345 Jul 26, 2019

larroy Jul 27, 2019

kshitij12345 Jul 30, 2019

larroy Jul 16, 2019

apeforest left a comment

karan6181 commented Aug 29, 2019

kshitij12345 commented Aug 30, 2019 •

edited

Loading

kshitij12345 commented Sep 11, 2019

kshitij12345 commented Sep 19, 2019

sxjscience commented Sep 19, 2019

kshitij12345 commented Sep 20, 2019

kshitij12345 commented Oct 20, 2019

larroy left a comment

apeforest left a comment

kshitij12345 commented Nov 22, 2019

[fix] missing input log higher order. #15331

[fix] missing input log higher order. #15331

Conversation

kshitij12345 commented Jun 23, 2019

roywei commented Jul 8, 2019

larroy commented Jul 9, 2019 • edited Loading

apeforest commented Jul 10, 2019

apeforest commented Jul 10, 2019

apeforest commented Jul 10, 2019 • edited Loading

apeforest commented Jul 10, 2019

kshitij12345 commented Jul 11, 2019

larroy commented Jul 16, 2019

larroy Jul 16, 2019

Choose a reason for hiding this comment

kshitij12345 Jul 17, 2019

Choose a reason for hiding this comment

larroy Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

kshitij12345 Jul 26, 2019

Choose a reason for hiding this comment

larroy Jul 27, 2019

Choose a reason for hiding this comment

kshitij12345 Jul 30, 2019

Choose a reason for hiding this comment

larroy Jul 16, 2019

Choose a reason for hiding this comment

apeforest left a comment

Choose a reason for hiding this comment

karan6181 commented Aug 29, 2019

kshitij12345 commented Aug 30, 2019 • edited Loading

kshitij12345 commented Sep 11, 2019

kshitij12345 commented Sep 19, 2019

sxjscience commented Sep 19, 2019

kshitij12345 commented Sep 20, 2019

kshitij12345 commented Oct 20, 2019

larroy left a comment

Choose a reason for hiding this comment

apeforest left a comment

Choose a reason for hiding this comment

kshitij12345 commented Nov 22, 2019

larroy commented Jul 9, 2019 •

edited

Loading

apeforest commented Jul 10, 2019 •

edited

Loading

larroy Jul 25, 2019 •

edited

Loading

kshitij12345 commented Aug 30, 2019 •

edited

Loading