add box coder op by Noplz · Pull Request #7922 · PaddlePaddle/Paddle

Noplz · 2018-01-28T07:43:06Z

qingqing01

Only part of the code is reviewed.

qingqing01 · 2018-01-31T09:15:22Z

@@ -0,0 +1,106 @@
+/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.


2016 -> 2018.

qingqing01 · 2018-01-31T09:16:44Z

+    AddInput(
+        "PriorBox",
+        "(Tensor, default Tensor<float>) "
+        "Box list PriorBox is a 2-D Tensor with shape [M, 4] holds N boxes, "


N boxes -> M boxes

qingqing01 · 2018-01-31T09:19:13Z

+        "coordinate of the anchor box.");
+    AddInput("PriorBoxVar",
+             "(Tensor, default Tensor<float>) "
+             "PriorBoxVar is a 2-D Tensor with shape [M, 4] holds N group "


M, 4] holds N group -> M, 4] holds M group

qingqing01 · 2018-01-31T09:24:45Z

+    auto prior_box_var_dims = ctx->GetInputDim("PriorBoxVar");
+    auto target_box_dims = ctx->GetInputDim("TargetBox");
+
+    PADDLE_ENFORCE_EQ(prior_box_dims.size(), 2UL,


The type of prior_box_dims.size() is int, comparing with 2UL will generate warnings. Just 2 is ok. The same as below.

qingqing01 · 2018-01-31T09:27:40Z

+    PADDLE_ENFORCE_EQ(prior_box_var_dims.size(), 2UL,
+                      "The rank of Input of PriorBoxVar must be 2");
+    PADDLE_ENFORCE_EQ(prior_box_var_dims[1], 4UL,
+                      "The shape of PriorBoxVar is [N, 4]");


change line 38 - line 41 to:

PADDLE_ENFORCE_EQ(prior_box_dims, prior_box_var_dims);

qingqing01 · 2018-01-31T09:38:45Z

+
+    AddComment(R"DOC(
+Bounding Box Coder Operator.
+Encode/Decode the priorbox information with the target bounding box.


Need more detailed description for encode and decode.

qingqing01 · 2018-01-31T09:42:28Z

+    PADDLE_ENFORCE_EQ(prior_box_var.dims().size(), 2,
+                      "The rank of prior_box_var must be 2.");
+    PADDLE_ENFORCE_EQ(prior_box.dims()[0], prior_box_var.dims()[0],
+                      "The dims of prior_box must equal to prior_box_var.");


All these checks have been done in the InferShape, can be removed here.

qingqing01 · 2018-01-31T09:44:54Z

+    int64_t col = prior_box.dims()[0];
+    auto* target_box_data = target_box.data<T>();
+    auto* prior_box_data = prior_box.data<T>();
+    auto* prior_box_var_data = prior_box_var.data<T>();


For short name, maybe:

target_box_data -> tbd
prior_box_data -> pbd
prior_box_var_data -> pbv

? I'm not sure, shortly name make code more clearly.

@pkuyym @wanghaox any advice?

qingqing01 · 2018-01-31T09:53:34Z

+
+    for (int64_t i = 0; i < row; ++i) {
+      for (int64_t j = 0; j < col; ++j) {
+        T prior_box_width = prior_box_data[j * 4 + 2] - prior_box_data[j * 4];


Do not use 4 directly, 4 can be got by the column of prior_box.

qingqing01 · 2018-01-31T09:53:50Z

+    PADDLE_ENFORCE_EQ(prior_box_var.dims().size(), 2,
+                      "The rank of prior_box_var must be 2.");
+    PADDLE_ENFORCE_EQ(prior_box.dims()[0], prior_box_var.dims()[0],
+                      "The dims of prior_box must equal to prior_box_var.");


Same with above.

qingqing01 · 2018-01-31T12:54:41Z

+namespace paddle {
+namespace operators {
+
+using platform::PADDLE_CUDA_NUM_THREADS;


It seems PADDLE_CUDA_NUM_THREADS is not used.

qingqing01 · 2018-01-31T13:00:25Z

+            output_box[i,:,2] = np.log(np.fabs(target_box_width[i] / prior_box_width)) / \
+                                prior_box_var[:,2]
+            output_box[i,:,3] = np.log(np.fabs(target_box_height[i] / prior_box_height)) / \
+                                prior_box_var[:,3]


Maybe short naming makes the code clearly.

另外，这里也可以看下是否可以用向量直接操作，而不是 for i in range(target_box.shape[0]):

向量操作改完了，名字的话和上面的最后看情况一起改吧

qingqing01

Almost LGTM.

qingqing01 · 2018-02-02T02:55:15Z

+                   "Input(PriorBox) of BoxCoderOp should not be null.");
+    PADDLE_ENFORCE(ctx->HasInput("PriorBoxVar"),
+                   "Input(PriorBoxVar) of BoxCoderOp should not be null.");
+    PADDLE_ENFORCE(ctx->HasInput("PriorBox"),


PADDLE_ENFORCE(ctx->HasInput("TargetBox"),

qingqing01 · 2018-02-02T02:55:58Z

+    PADDLE_ENFORCE(ctx->HasInput("PriorBoxVar"),
+                   "Input(PriorBoxVar) of BoxCoderOp should not be null.");
+    PADDLE_ENFORCE(ctx->HasInput("PriorBox"),
+                   "Input(TargetBox) of BoxCoderOp should not be null.");


Also need to check output var:

PADDLE_ENFORCE(ctx->HasOutput("OutputBox"), ...);

qingqing01 · 2018-02-02T03:07:38Z

+    def setUp(self):
+        self.op_type = "box_coder"
+        lod = [[0, 20]]
+        prior_box = np.random.random((10, 4)).astype('float32')


Actually, [xmin, ymin, xmax, ymax] in prior box should follow: xmin < xmax, ymin < ymax. 这里，我觉得等box_coder, hard_example_mine, nms_op merge之后统一改下，写成一个函数，这几个op的单测复用。这里随机生成，测正确性没有问题，就暂时不改了。

qingqing01 · 2018-02-02T03:30:38Z

+namespace operators {
+
+using Tensor = framework::Tensor;
+using LoDTensor = framework::LoDTensor;


Remove these two lines, use framework::Tensor and framework::LoDTensordirectly in following code.

qingqing01

LGTM.

gaoyuan and others added 2 commits January 28, 2018 15:38

add box coder op

72eccb2

Update box_coder_op.cc

bc6c4db

qingqing01 requested review from pkuyym, qingqing01 and wanghaox January 29, 2018 02:14

qingqing01 reviewed Jan 31, 2018

View reviewed changes

gaoyuan added 4 commits January 31, 2018 21:32

update according to the code review

58bfaea

update according to the code review

02d2b7b

update accoding to the code review

c3e89f3

update accoding to the code review

e14272b

qingqing01 reviewed Feb 2, 2018

View reviewed changes

gaoyuan added 2 commits February 2, 2018 13:35

Update according to the code review

251c2fd

Update according to the code review

7d8d9db

qingqing01 approved these changes Feb 2, 2018

View reviewed changes

qingqing01 mentioned this pull request Feb 2, 2018

The TODO lists for MobileNet-SSD model. #7488

Closed

25 tasks

qingqing01 merged commit b7db353 into PaddlePaddle:develop Feb 2, 2018

		@@ -0,0 +1,106 @@
		/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.

Uh oh!

Conversation

Noplz commented Jan 28, 2018

Uh oh!

qingqing01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Noplz Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingqing01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingqing01 Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingqing01 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Noplz Jan 31, 2018 •

edited

Loading

qingqing01 Feb 2, 2018 •

edited

Loading