-
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
detection map evaluator for SSD #6588
Conversation
namespace operators { | ||
namespace math {} // namespace math | ||
} // namespace operators | ||
} // namespace paddle |
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.
我想这个文件是可以不要的
namespace operators { | ||
namespace math {} // namespace math | ||
} // namespace operators | ||
} // namespace paddle |
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.
同上
return pair1.first > pair2.first; | ||
} | ||
|
||
// template <> |
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.
这段注释可以去掉吗?
paddle/operators/detection_map_op.cc
Outdated
Detection MAP evaluator for SSD(Single Shot MultiBox Detector) algorithm. | ||
Please get more information from the following papers: | ||
https://arxiv.org/abs/1512.02325. | ||
|
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.
建议增加一点更详细的内容
paddle/operators/detection_map_op.cc
Outdated
framework::OpKernelType GetExpectedKernelType( | ||
const framework::ExecutionContext& ctx) const override { | ||
return framework::OpKernelType( | ||
framework::ToDataType(ctx.Input<framework::Tensor>("Label")->type()), |
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.
The kernel type (float or double) should be deduced by Detection
.
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/detection_map_op.cc
Outdated
public: | ||
DetectionMAPOpMaker(OpProto* proto, OpAttrChecker* op_checker) | ||
: OpProtoAndCheckerMaker(proto, op_checker) { | ||
AddInput("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.
在box_coder_op, target_assign_op中,假定:ground-truth box和ground-truth label是两个输入。 这里是concat到一起,合成了一个输入。这点需要讨论下?
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.
建议在python封装中concat在一起
paddle/operators/detection_map_op.cc
Outdated
|
||
auto ap_type = GetAPType(ctx->Attrs().Get<std::string>("ap_type")); | ||
PADDLE_ENFORCE_NE(ap_type, APType::kNone, | ||
"The ap_type should be 'integral' or '11point."); |
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.
Put the check for attr in the DetectionMAPOpMaker
.
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,moved to DetectionMAPOpMaker.
paddle/operators/detection_map_op.h
Outdated
size_t num = tp_sum.size(); | ||
// Compute Precision. | ||
for (size_t i = 0; i < num; ++i) { | ||
// CHECK_LE(tpCumSum[i], labelNumPos); |
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 comment this line?
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/detection_map_op.h
Outdated
} | ||
} | ||
|
||
T CalcMAP( |
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.
这个函数只计算了当前mini-batch的mAP。 多mini-batch累积计算mAP时需要label_pos_count, true_pos, false_pos,目前Fluid是在Python里用其他op组合计算多mini-batch累积的。 因此这3个是需要输出的,但目前std::map<int, std::vector<std::pair<T, int>>>
数据结构无法作为op的输出。
这块需要讨论下~
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,增加了OutPosCount、OutTruePos、OutFalsePos的输出
paddle/operators/detection_map_op.cc
Outdated
@@ -0,0 +1,123 @@ | |||
/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. |
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.
2016 => 2018
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/detection_map_op.h
Outdated
@@ -0,0 +1,329 @@ | |||
/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. |
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.
2016 => 2018
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
if (false_pos_data[j * 2 + 1] < kEPS) flag = 0; | ||
false_pos[i].push_back(std::make_pair(score, flag)); | ||
} | ||
} |
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.
true_pos 和 false_pos赋值代码一样,可以用lambda函数写,简化代码。
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
|
||
GetBoxes(*in_label, *in_detect, gt_boxes, detect_boxes); | ||
|
||
std::map<int, int> label_pos_count; |
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.
label_pos_count没有必要定义成 std::map<int, int>
,可以直接用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.
代码改动量较大,作为后续优化点改进
continue; | ||
} | ||
int label = it->first; | ||
if (label_pos_count.find(label) == label_pos_count.end()) { |
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.
label_pos_count改成Tensor,先初始化下,这里就不用if
分支。
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.
label_pos_count改成Tensor代码改动量较大,作为后续优化点改进
"no detected data."); | ||
AddInput("PosCount", | ||
"(Tensor) A tensor with shape [Ncls, 1], store the " | ||
"input positive example count of each class.") |
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.
不够好理解,看doc并不知道这个input是干啥的。
下面doc注释存在同样问题。
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.
Agree
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,已增加comment
"(LodTensor) A LodTensor with shape [Nfp', 2], store the " | ||
"false positive example of each class. It combines the " | ||
"input(FalsePos) and the false positive examples computed from " | ||
"input(Detection) and input(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.
- OutPosCount, OutTruePos, OutFalsePos 看注释这3个输出,包含previous mini-batch的状态。
- 这个op只能算累计的mAP,无法算当前mini-batch的mAP?
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.
1,包含previous mini-batch的状态
2,当PosCount等传入为空,即只计算当前mini-batch的mAP
已增加注释
using framework::OperatorWithKernel::OperatorWithKernel; | ||
|
||
void InferShape(framework::InferShapeContext* ctx) const override { | ||
PADDLE_ENFORCE(ctx->HasInput("Detection"), |
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 DetectRes
more suitable ?
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
"instance, the offsets in first dimension are called LoD, " | ||
"the number of offset is N + 1, if LoD[i + 1] - LoD[i] == 0, " | ||
"means there is no ground-truth data."); | ||
AddInput("Detection", |
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 exchange the position of Detection
and 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
"no detected data."); | ||
AddInput("PosCount", | ||
"(Tensor) A tensor with shape [Ncls, 1], store the " | ||
"input positive example count of each class.") |
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.
Agree
"input positive example count of each class.") | ||
.AsDispensable(); | ||
AddInput("TruePos", | ||
"(LodTensor) A 2-D LodTensor with shape [Ntp, 2], store the " |
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.
LodTensor --> LoDTensor
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 InTruePos
is 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,
好像其他op的输入参数,较少用In开头的, 没有修改为InTruePos
"(LodTensor) A 2-D LodTensor with shape [Nfp, 2], store the " | ||
"input false positive example of each class.") | ||
.AsDispensable(); | ||
AddOutput("OutPosCount", |
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 AccumPosCount
more suitable?
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
.SetDefault(.3f); | ||
AddAttr<bool>("evaluate_difficult", | ||
"(bool, default true) " | ||
"Switch to control whether the difficult data is evaluated.") |
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.
control --> decide
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.
有什么区别??
"(string, default 'integral') " | ||
"The AP algorithm type, 'integral' or '11point'.") | ||
.SetDefault("integral") | ||
.InEnum({"integral", "11point"}) |
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 it's better to declare two const variables ?
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 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.
我看头文件里面有声明enum类型的APType,直接用GetAPType(APType. kIntegral)可以避免"integral","11point"四处出现,因为程序里面用到的type是enum型的
AddComment(R"DOC( | ||
Detection mAP evaluate operator. | ||
The general steps are as follows. First, calculate the true positive and | ||
false positive according to the input of detection and labels, then |
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.
true positive you mean count of true positive ?
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.
not the count of true positive
|
||
mAP += average_precisions | ||
count += 1 | ||
self.out_class_pos_count, self.out_true_pos, self.out_true_pos_lod, self.out_false_pos, self.out_false_pos_lod = get_output_pos( |
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.
Seem exceeds 80 columns.
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.
223-162=61 行
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 mean columns
mAP += average_precisions; | ||
++count; | ||
} else { | ||
LOG(FATAL) << "Unkown ap version: " << ap_type; |
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 better to throw a exception here
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.
已经进行过参数检查,不会运行到这里,这个else可以删除,保留只是为了代码分枝完备
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 approve this PR. In order to train model, we ensure the correctness of the computational logic of mAP at first. In the future, we will refine this operator.
resolve #6266