Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

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

Merged
apeforest merged 12 commits intoapache:masterfrom
kshitij12345:develop/add-higher-order/arctan-arctanh
Sep 6, 2019
Merged

[MXNET-978] Higher Order Gradient Support arctan, arctanh, radians.#15531
apeforest merged 12 commits intoapache:masterfrom
kshitij12345:develop/add-higher-order/arctan-arctanh

Conversation

@kshitij12345
Copy link
Contributor

@kshitij12345 kshitij12345 commented Jul 13, 2019 •
edited
Loading

Description

PR intends to add support for higher order gradient for arctan, arctanh, radians.

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 arctan, arctanh, radians.
  • unit test for the same.

@kshitij12345 kshitij12345 changed the title [MXNET-978] Higher Order Gradient Support arctan, arctanh. [MXNET-978] Higher Order Gradient Support arctan, arctanh, radians. Jul 14, 2019
marcoabreu
marcoabreu approved these changes Jul 14, 2019
View reviewed changes
Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests lgtm

kshitij12345
kshitij12345 commented Jul 15, 2019
View reviewed changes
src/operator/tensor/util/node_op_util.h Outdated
namespace mxnet {
namespace util {

class NodeOp {
Copy link
Contributor Author

@kshitij12345 kshitij12345 Jul 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@larroy @apeforest @marcoabreu ,
Have added this class as an abstraction over MakeNode. Using this we can avoid #15331 , as missing argument will be detected at compile time. Also it reduces the noise while using MakeNode. There shouldn't be any performance hit as the member functions should get inlined.

arctan and arctanh both compute roughly the same thing. Can see the difference by using this class as abstraction and the our usual method. arctan -> Usual :: arctanh -> NodeOp

It might be possible that I have missed something.

Let me know what you think, will move forward from there or reset the commit.
Thanks.

Copy link
Contributor

@larroy larroy Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, indeed reduces noise.

Copy link
Contributor Author

@kshitij12345 kshitij12345 Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'll also update arctan to use this.
Thanks.

Copy link
Contributor Author

@kshitij12345 kshitij12345 Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apeforest Do review this part. If it looks good, I'll update all other PRs to use this machinery once this is in.

@karan6181
Copy link
Contributor

karan6181 commented Jul 15, 2019

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

@marcoabreu marcoabreu added Operator pr-awaiting-review PR is waiting for code review labels Jul 15, 2019
larroy
larroy approved these changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@larroy larroy left a 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, LGTM, very clean.

@larroy
Copy link
Contributor

larroy commented Jul 16, 2019

Shall we rename grad_x to x_grad for consistency with other operators and PRs? What do you think?

@kshitij12345
Copy link
Contributor Author

kshitij12345 commented Jul 17, 2019

For some reason I thought, it was grad_x, but right x_grad makes sense for consistency. Will update it.

@kshitij12345
changes
97b5fb7 
* use NodeOp for arctan.
* update few comments.
* update few variable names.
@kshitij12345
rename grad_x to x_grad
8b8ab2a
larroy
larroy approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kshitij12345 kshitij12345 mentioned this pull request Jul 19, 2019
Merged
7 tasks
apeforest
apeforest reviewed Jul 19, 2019
View reviewed changes
apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
apeforest
apeforest reviewed Aug 27, 2019
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Looks good overall. Please address reviewer comments and retrigger CI. The CI system has been unstable recently. Sorry for the inconvenience. We are working on it :)

@kshitij12345
address comments
ef01b9f 
* rename NodeOp to NodeOpGen.
* rename Op to op.
kshitij12345
kshitij12345 commented Aug 28, 2019
View reviewed changes
This was referenced Aug 30, 2019
Merged
Merged
Merged
Closed
@kshitij12345
Copy link
Contributor Author

kshitij12345 commented Aug 30, 2019

@larroy @apeforest Could you please review it again.

@kshitij12345
Copy link
Contributor Author

kshitij12345 commented Sep 6, 2019

@apeforest @larroy Gentle ping. Could you please review it again?

@apeforest apeforest merged commit 255dff0 into apache:master Sep 6, 2019
@kshitij12345 kshitij12345 deleted the develop/add-higher-order/arctan-arctanh branch September 6, 2019 19:29
gyshi pushed a commit to gyshi/incubator-mxnet that referenced this pull request Sep 7, 2019
@kshitij12345 @gyshi
[MXNET-978] Higher Order Gradient Support arctan, arctanh, `radia…
181eb0f 
…ns`. (apache#15531)

* support arc{tan/tanh} for higher order grad

* add relevant tests

* add new abstraction for Node operations

* support radians for higher order grad

* add test for radians

* changes

* use NodeOp for arctan.
* update few comments.
* update few variable names.

* rename grad_x to x_grad

* update comments

* move node_op_util.h to src/nnvm

* address comments

* rename NodeOp to NodeOpGen.
* rename Op to op.

* fix file description
gyshi pushed a commit to gyshi/incubator-mxnet that referenced this pull request Sep 7, 2019
@kshitij12345 @gyshi
[MXNET-978] Higher Order Gradient Support arctan, arctanh, `radia…
0c3d44d 
…ns`. (apache#15531)

* support arc{tan/tanh} for higher order grad

* add relevant tests

* add new abstraction for Node operations

* support radians for higher order grad

* add test for radians

* changes

* use NodeOp for arctan.
* update few comments.
* update few variable names.

* rename grad_x to x_grad

* update comments

* move node_op_util.h to src/nnvm

* address comments

* rename NodeOp to NodeOpGen.
* rename Op to op.

* fix file description
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@apeforest apeforest Awaiting requested review from apeforest

2 more reviewers

@larroy larroy larroy approved these changes

@marcoabreu marcoabreu marcoabreu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Operator pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kshitij12345 @karan6181 @larroy @apeforest @marcoabreu