-
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 greater than and less than equal ops to compare op #5609
Conversation
paddle/operators/compare_op.cc
Outdated
@@ -94,5 +94,14 @@ class CompareOp : public framework::OperatorWithKernel { | |||
|
|||
REGISTER_LOGICAL_OP(less_than, "Out = X < Y"); | |||
REGISTER_LOGICAL_KERNEL(less_than, CPU, paddle::operators::LessThanFunctor); | |||
REGISTER_LOGICAL_OP(less_than_equal, "Out = X <= Y"); |
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.
less_equal better? LE for short
paddle/operators/compare_op.cc
Outdated
REGISTER_LOGICAL_OP(greater_than, "Out = X > Y"); | ||
REGISTER_LOGICAL_KERNEL(greater_than, CPU, | ||
paddle::operators::GreaterThanFunctor); | ||
REGISTER_LOGICAL_OP(greater_than_equal, "Out = X >= Y"); |
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.
greater_equal
I find that the names in Tensorflow's comparison operators are more clear
https://www.tensorflow.org/versions/r0.12/api_docs/python/control_flow_ops/comparison_operators
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.
Even I prefer the names in Tensorflow. However, I kept this because we already had the less_than
op. Do you think I should refactor all of them to have easy and simple names like Tensorflow?
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 think that the Tensorflow's name is better, shorter but not confusing.
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.
Updated in the latest commit.
paddle/operators/compare_op.cu
Outdated
@@ -15,4 +15,10 @@ | |||
#include "paddle/operators/compare_op.h" | |||
|
|||
REGISTER_LOGICAL_KERNEL(less_than, GPU, paddle::operators::LessThanFunctor); | |||
REGISTER_LOGICAL_KERNEL(less_equal, GPU, | |||
paddle::operators::LessThanEqualFunctor); |
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.
should these functors be changed too?
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.
Yes. Thank you for pointing that out. Changed it in the latest commit.
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
No description provided.