-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-978] Higher Order Gradient Support arctan
, arctanh
, radians
.
#15531
[MXNET-978] Higher Order Gradient Support arctan
, arctanh
, radians
.
#15531
Conversation
arctan
, arctanh
.arctan
, arctanh
, radians
.
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.
Tests lgtm
namespace mxnet { | ||
namespace util { | ||
|
||
class NodeOp { |
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.
@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.
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.
Looks good to me, indeed reduces noise.
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.
In that case, I'll also update arctan
to use this.
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.
@apeforest Do review this part. If it looks good, I'll update all other PRs to use this machinery once this is in.
@mxnet-label-bot add [Operator, pr-awaiting-review] |
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, LGTM, very clean.
Shall we rename grad_x to x_grad for consistency with other operators and PRs? What do you think? |
For some reason I thought, it was |
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.
LGTM
…to develop/add-higher-order/arctan-arctanh
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 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 :)
* rename NodeOp to NodeOpGen. * rename Op to op.
@larroy @apeforest Could you please review it again. |
@apeforest @larroy Gentle ping. Could you please review it again? |
…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
…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
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.
Changes
arctan
,arctanh
,radians
.