[MXNET-978] Higher Order Gradient Support `arcsin`, `arccos`. #15515

kshitij12345 · 2019-07-11T14:27:53Z

Description

PR intends to add support for higher order gradient for arcsin, arccos.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA-978 issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

higher order gradient for a arcsin, arccos.
unit test for the same.

ChaiBapchya

Barring that hacky way of testing (which makes sense but I'll wait for committers to approve it), LGTM! Thanks for your contribution!

kshitij12345 · 2019-07-14T07:23:12Z

Thank You.
Do note that, the other alternative is 2.*(array - np.min(array))/np.ptp(array)-1, but we will need to handle the case with array of one element array.

karan6181 · 2019-07-16T22:57:29Z

@mxnet-label-bot add [Operator, pr-awaiting-review]

…to develop/add-higher-order/arcsin-arccos

kshitij12345 · 2019-10-02T12:07:26Z

@apeforest @larroy @sxjscience Gentle Ping for review.:)

larroy

Hi kshitij12345, thanks a lot for your contribution. In general looks good. One question regarding the first output of the second gradient.

larroy · 2019-10-02T20:22:22Z

src/operator/tensor/elemwise_unary_op_trig.cc

+      auto grad_grad_x = op.mul(dydx_mul_grad_x, grad_x_square_mul_x);
+
+      std::vector<nnvm::NodeEntry> ret;
+      ret.emplace_back(op.mul(ograds[0], grad_x));


if the first input is y_grad or dL/dy, this gradient should be dL/(dy*dx) ?
didn't we have the convention of x_grad instead of grad_x?

Sorry for the late reply.

if the first input is y_grad or dL/dy, this gradient should be dL/(dy*dx) ?

I am not sure of dL part because we don't really use it in computing the loss function.

didn't we have the convention of x_grad instead of grad_x?

Oops. Thanks. Was a old PR. Will update the names.

Sure, l guess calling it x_grad_y_grad is fine. Sorry CI is flaky now.

x_grad_y_grad we are not naming that particular variable anywhere. Or am I confusing something?

@larroy Can you elaborate on what you meant with x_grad_y_grad. I am slightly confused. Thanks.

* rename grad_x -> x_grad

kshitij12345 · 2019-12-12T16:50:57Z

@apeforest @larroy @sxjscience
Gentle Ping for review.

apeforest

LGTM!

larroy

LGTM.

If you have interest, one idea that circulated around to enhace higher order gradients is to add a "FGradientSymbolic" function that gets triggered when the higher order gradient is not avaiable and changes the graph to have the forward pass expressed in terms of differentiable primitives. We can talk more if you are interested.

kshitij12345 · 2019-12-21T08:42:24Z

@larroy , I am quite interested in the idea. What would be a good place to talk? Slack?

sxjscience · 2019-12-21T08:44:34Z

@larroy @kshitij12345 @apeforest I think we can use Slack. What do you think?

kshitij12345 · 2019-12-21T10:00:24Z

Slack sounds good to me.

apeforest · 2019-12-27T07:16:45Z

@kshitij12345 I have sent a slack invite to [email protected]. Please accept. Thanks!

larroy · 2020-01-04T01:17:07Z

Slack or mailing list are fine for me.

kshitij12345 added 3 commits July 11, 2019 19:46

support arcsin, arccos for higher order grad

9f35c99

add relevant tests

df3551b

add small note for computation

7daaf76

kshitij12345 force-pushed the develop/add-higher-order/arcsin-arccos branch from 4c45f2f to 7daaf76 Compare July 11, 2019 14:37

ChaiBapchya approved these changes Jul 12, 2019

View reviewed changes

marcoabreu added Operator pr-awaiting-review PR is waiting for code review labels Jul 16, 2019

kshitij12345 added 6 commits July 26, 2019 20:36

update comments

362632b

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

935da99

…to develop/add-higher-order/arcsin-arccos

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

0a0f071

…to develop/add-higher-order/arcsin-arccos

Merge latest 'master' into develop/add-higher-order/arcsin-arccos

a63c019

use NodeOpGen

4d77584

retrigger CI

5e2c3e6

larroy reviewed Oct 2, 2019

View reviewed changes

kshitij12345 added 3 commits October 9, 2019 21:22

address comment

743cc70

* rename grad_x -> x_grad

retrigger CI

211b3d4

retrigger CI

8b71be5

apeforest approved these changes Dec 12, 2019

View reviewed changes

larroy approved these changes Dec 18, 2019

View reviewed changes

sxjscience merged commit ed09547 into apache:master Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-978] Higher Order Gradient Support `arcsin`, `arccos`. #15515

[MXNET-978] Higher Order Gradient Support `arcsin`, `arccos`. #15515

kshitij12345 commented Jul 11, 2019

ChaiBapchya left a comment

kshitij12345 commented Jul 14, 2019 •

edited

Loading

karan6181 commented Jul 16, 2019

kshitij12345 commented Oct 2, 2019

larroy left a comment

larroy Oct 2, 2019

kshitij12345 Oct 9, 2019

larroy Oct 9, 2019

kshitij12345 Oct 13, 2019

kshitij12345 Oct 19, 2019

kshitij12345 commented Dec 12, 2019

apeforest left a comment

larroy left a comment •

edited

Loading

kshitij12345 commented Dec 21, 2019

sxjscience commented Dec 21, 2019

kshitij12345 commented Dec 21, 2019

apeforest commented Dec 27, 2019

larroy commented Jan 4, 2020

[MXNET-978] Higher Order Gradient Support arcsin, arccos. #15515

[MXNET-978] Higher Order Gradient Support arcsin, arccos. #15515

Conversation

kshitij12345 commented Jul 11, 2019

Description

Checklist

Essentials

Changes

ChaiBapchya left a comment

Choose a reason for hiding this comment

kshitij12345 commented Jul 14, 2019 • edited Loading

karan6181 commented Jul 16, 2019

kshitij12345 commented Oct 2, 2019

larroy left a comment

Choose a reason for hiding this comment

larroy Oct 2, 2019

Choose a reason for hiding this comment

kshitij12345 Oct 9, 2019

Choose a reason for hiding this comment

larroy Oct 9, 2019

Choose a reason for hiding this comment

kshitij12345 Oct 13, 2019

Choose a reason for hiding this comment

kshitij12345 Oct 19, 2019

Choose a reason for hiding this comment

kshitij12345 commented Dec 12, 2019

apeforest left a comment

Choose a reason for hiding this comment

larroy left a comment • edited Loading

Choose a reason for hiding this comment

kshitij12345 commented Dec 21, 2019

sxjscience commented Dec 21, 2019

kshitij12345 commented Dec 21, 2019

apeforest commented Dec 27, 2019

larroy commented Jan 4, 2020

[MXNET-978] Higher Order Gradient Support `arcsin`, `arccos`. #15515

[MXNET-978] Higher Order Gradient Support `arcsin`, `arccos`. #15515

kshitij12345 commented Jul 14, 2019 •

edited

Loading

larroy left a comment •

edited

Loading