-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-853] Fix for smooth_l1 operator scalar default value #12284
Conversation
It should properly catch the exception anyway, rather than assigning every single operator a default value |
.set_attr<nnvm::FGradient>("FGradient", ElemwiseGradUseIn{ "_backward_smooth_l1" }); | ||
|
||
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.
Why extra 2 blank spaces here?
.set_attr<nnvm::FInplaceOption>("FInplaceOption", | ||
[](const NodeAttrs& attrs){ | ||
return std::vector<std::pair<int, int> >{{0, 0}}; | ||
}) |
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.
seems like you're using tabs here, please switch to spaces.
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.
sorry, new editor not setup properly....
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.
Please fix all lint issues first: http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12284/2/pipeline
@haojin2 linting fixed |
@samskalicky I didn't mean not sending this PR to fix the problem, but should address the exception catch problem anyways. Sorry about the confusion. #12286 is a perfect attempt. Thanks! This LGTM now. |
@anirudh2290 can you help review/merge? |
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. In future, we would want to add a dmlc::Parameter for ops like smooth_l1. This would help with improvement of operator documentation too. Currently the default value won't get documented.
they do not provide enough support for checking for arguments and setting custom default values
0f00c2a
to
756cc80
Compare
@anirudh2290 fixed syntax to match mxnet style (curly braces). This change is now ready for merging |
…2284) * changed smooth_l1 operator implementation to not use helper macros since they do not provide enough support for checking for arguments and setting custom default values * added testcase for smooth_l1 operator scalar default value * fixed whitespace * added curly braces for if/else to match mxnet style * added more curly braces
…2284) * changed smooth_l1 operator implementation to not use helper macros since they do not provide enough support for checking for arguments and setting custom default values * added testcase for smooth_l1 operator scalar default value * fixed whitespace * added curly braces for if/else to match mxnet style * added more curly braces
Description
Fix for issue #7533 Call symbol/nd.smooth_l1 without scalar causing uncaught exception
changed smooth_l1 operator implementation to not use helper macros since
they do not provide enough support for checking for arguments and
setting custom default values
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
changed smooth_l1 operator implementation to not use helper macros since
they do not provide enough support for checking for arguments and
setting custom default values
Comments