-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding Hard Sigmoid Activation #4771
Adding Hard Sigmoid Activation #4771
Conversation
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, one minor comment.
This is much faster than sigmoid. | ||
|
||
hard_sigmoid = max(0, min(1, slope * x + shift)) | ||
|
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.
Is it true that both slope and shift are non-negative? If yes, it will be helpful to mention as a comment.
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.
Fixed in f4820c7
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.
Corrected comment grammatical mistake in 7b10546
framework::OpAttrChecker *op_checker) | ||
: OpProtoAndCheckerMaker(proto, op_checker) { | ||
AddInput("X", "Input of HardSigmoid operator"); | ||
AddOutput("Y", "Output of HardSigmoid operator"); |
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.
Out name convention documentation suggest us use the "(type, default value) usage" style to write the comments. I guess this comment can be further refined. And fc_op is a good example: https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/fc_op.cc#L122.
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.
@lcy-seso Do you recommend that I modify this for all ops in the activation.cc? Unfortunately all activations in this file do not follow the naming convention. The other option is that I merge this PR and open an issue to fix the naming convention of all Ops in Activations? The second option will keep this PR clean. What do you suggest?
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.
I prefer we guarantee all the newly written unmerged PR follows this naming convention first, and make a new PR later to fix all the old codes.
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.
Thank you for pointing this out. Opened a separate issue to fix this. #4772
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.
Fixes #4605