-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Auc op #4063
Auc op #4063
Conversation
paddle/operators/auc_op.cc
Outdated
// TODO(typhoonzero): support weight input | ||
AddOutput("AUC", | ||
"A scalar `Tensor` representing the " | ||
"current area-under-curve."); |
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 it's better to tell users "what is it" before "how to represent it".
So the comment might be better to write "Current AUC(area-under-curve), represented by a scalar Tensor
."
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 it's better to tell users "what is it" before "how to represent it".
Correct, but in this line, "what is it", the output is a scalar indeed, and "representing ..." is what it does.
paddle/operators/auc_op.cc
Outdated
"A scalar `Tensor` representing the " | ||
"current area-under-curve."); | ||
|
||
AddAttr<std::string>("curve", "Possible curves are ROC and PR") |
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.
"Curves type, can be 'ROC' or 'PR'."
seems better?
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.
Done.
paddle/operators/auc_op.cc
Outdated
AucOpMaker(framework::OpProto *proto, framework::OpAttrChecker *op_checker) | ||
: OpProtoAndCheckerMaker(proto, op_checker) { | ||
AddInput("Inference", | ||
"A floating point `Tensor` of arbitrary shape and whose values" |
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.
There is no need to surround Tensor with ``, because this comment will be read by Python users and they know nothing about C++ class Tensor. So tensor is OK.
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.
Done.
paddle/operators/auc_op.cc
Outdated
PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("Inference"), | ||
"Input of Inference must be initialized."); | ||
PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("Label"), | ||
"Input of Inference must be initialized."); |
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.
Input or Label?
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.
Done.
float* tp_rate_data = tp_rate.mutable_data<float>(ctx.GetPlace()); | ||
float* fp_rate_data = fp_rate.mutable_data<float>(ctx.GetPlace()); | ||
float* rec_rate_data = rec_rate.mutable_data<float>(ctx.GetPlace()); | ||
for (int i = 0; i < num_thresholds; i++) { |
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.
Maybe we can convert Tensor to Eigen::Tensor, and do vector computation instead of loop.
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.
Added to TODO, will refine if eigen have enough 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.
Maybe https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/elementwise_div_op.h#L71
can be used as a reference.
auto* label = ctx.Input<Tensor>("Label"); | ||
auto* auc = ctx.Output<Tensor>("AUC"); | ||
|
||
float* auc_data = auc->mutable_data<float>(ctx.GetPlace()); |
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.
float or T?
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.
Evaluator output is always float.
for (int i = 1; i < num_thresholds - 1; i++) { | ||
thresholds_list[i] = (float)i / (num_thresholds - 1); | ||
} | ||
const float kEpsilon = 1e-7; |
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.
Overflow the accuracy range?
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 didn't get your point?
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.
Oh, maybe float type can have 7 significant digits, I'm not sure about this.
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
Fix #4062
This is WIP, will add GPU code in next PR.