prior box operator for ssd by wanghaox · Pull Request #6150 · PaddlePaddle/Paddle

wanghaox · 2017-12-01T06:28:23Z

resolve #6015

pkuyym

Maybe we can make PriorBoxOp more general to support both Faster RCNN and SSD, please refer the design of TensorFlow. Thanks.

pkuyym · 2017-12-06T03:18:40Z

paddle/operators/prior_box_op.cc

+
+  void InferShape(framework::InferShapeContext* ctx) const override {
+    PADDLE_ENFORCE(ctx->HasInput("Input"),
+                   "Input(X) of SequenceSliceOp should not be null.");


Please check this comment.

pkuyym · 2017-12-06T03:18:54Z

paddle/operators/prior_box_op.cc

+    PADDLE_ENFORCE(ctx->HasInput("Input"),
+                   "Input(X) of SequenceSliceOp should not be null.");
+    PADDLE_ENFORCE(ctx->HasInput("Image"),
+                   "Input(Offset) of SequenceSliceOp should not be null.");


Same as above.

I think we only use the shape of image. Is it necessary to pass the full image ?

done
I think to pass the full image is easy for the user to use, and maybe easy to update algo. And the

pkuyym · 2017-12-06T03:22:07Z

paddle/operators/prior_box_op.cc

+                   "The format of input tensor is NCHW.");
+
+    auto min_sizes = ctx->Attrs().Get<std::vector<int>>("min_sizes");
+    auto max_sizes = ctx->Attrs().Get<std::vector<int>>("max_sizes");


Any possible to make 'max_sizes' optional?

add SetDefault({}) to max_sizes at line 126

pkuyym · 2017-12-06T03:24:25Z

paddle/operators/prior_box_op.cc

+        ctx->Attrs().Get<std::vector<float>>("aspect_ratios");
+    bool flip = ctx->Attrs().Get<bool>("flip");
+
+    PADDLE_ENFORCE_GT(min_sizes.size(), 0, "must provide min_size.");


Please refine this comment and make sure the first character is upper-case.
I think 'Size of min_size must be at least 1.' is more accurate.

pkuyym · 2017-12-06T03:37:05Z

paddle/operators/prior_box_op.cc

+    PADDLE_ENFORCE_GT(step_w, 0.0, "step_w should be larger than 0.");
+
+    const int layer_height = input_dims[3];
+    const int layer_width = input_dims[2];


The shape of input is NCHW or NCWH?

pkuyym · 2017-12-06T03:40:17Z

paddle/operators/prior_box_op.cc

+    dim_vec[2] = layer_width * layer_height * num_priors * 4;
+    PADDLE_ENFORCE_GT(dim_vec[2], 0,
+                      "output_dim[2] must larger than 0."
+                      "check your data dims");


If possible, please find the illegal input. For example, layer_width is illegal or layer_height is illegal etc.

done, add PADDLE_ENFORCE(input_dims.size() == 4) and check layer_width or layer_height is smaller than image's.

pkuyym · 2017-12-06T03:40:49Z

paddle/operators/prior_box_op.cc

+                  framework::OpAttrChecker* op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("Input",
+             "(Tensor), "


Please give type info like default Tensor<float>.

pkuyym · 2017-12-06T03:41:28Z

paddle/operators/prior_box_op.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("Input",
+             "(Tensor), "
+             "the input feature data of PriorBoxOp.");


Lack shape information and no need to start in a newline.

wanghaox

update code

wanghaox · 2017-12-13T05:56:43Z

paddle/operators/prior_box_op.cc

+
+  void InferShape(framework::InferShapeContext* ctx) const override {
+    PADDLE_ENFORCE(ctx->HasInput("Input"),
+                   "Input(X) of SequenceSliceOp should not be null.");


wanghaox · 2017-12-13T05:59:14Z

paddle/operators/prior_box_op.cc

+    PADDLE_ENFORCE(ctx->HasInput("Input"),
+                   "Input(X) of SequenceSliceOp should not be null.");
+    PADDLE_ENFORCE(ctx->HasInput("Image"),
+                   "Input(Offset) of SequenceSliceOp should not be null.");


done
I think to pass the full image is easy for the user to use, and maybe easy to update algo. And the

wanghaox · 2017-12-13T05:59:53Z

paddle/operators/prior_box_op.cc

+                   "The format of input tensor is NCHW.");
+
+    auto min_sizes = ctx->Attrs().Get<std::vector<int>>("min_sizes");
+    auto max_sizes = ctx->Attrs().Get<std::vector<int>>("max_sizes");


add SetDefault({}) to max_sizes at line 126

wanghaox · 2017-12-13T06:00:44Z

paddle/operators/prior_box_op.cc

+        ctx->Attrs().Get<std::vector<float>>("aspect_ratios");
+    bool flip = ctx->Attrs().Get<bool>("flip");
+
+    PADDLE_ENFORCE_GT(min_sizes.size(), 0, "must provide min_size.");


wanghaox · 2017-12-13T06:25:58Z

paddle/operators/prior_box_op.cc

+    PADDLE_ENFORCE_GT(step_w, 0.0, "step_w should be larger than 0.");
+
+    const int layer_height = input_dims[3];
+    const int layer_width = input_dims[2];


wanghaox · 2017-12-13T06:37:06Z

paddle/operators/prior_box_op.cc

+    dim_vec[2] = layer_width * layer_height * num_priors * 4;
+    PADDLE_ENFORCE_GT(dim_vec[2], 0,
+                      "output_dim[2] must larger than 0."
+                      "check your data dims");


done, add PADDLE_ENFORCE(input_dims.size() == 4) and check layer_width or layer_height is smaller than image's.

wanghaox · 2017-12-13T06:38:53Z

paddle/operators/prior_box_op.cc

+                  framework::OpAttrChecker* op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("Input",
+             "(Tensor), "


wanghaox · 2017-12-13T06:40:52Z

paddle/operators/prior_box_op.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("Input",
+             "(Tensor), "
+             "the input feature data of PriorBoxOp.");


change at line 34

sweetsky0901 · 2018-01-03T06:48:54Z

paddle/operators/prior_box_op.h

+inline void expand_aspect_ratios(const std::vector<float> input_aspect_ratior,
+                                 bool flip,
+                                 std::vector<float>& output_aspect_ratior) {
+  constexpr float eps = 1e-6;


eps建议epsilon，在重构后的版本中看到好像都用这个词。
另外这个建议做成参数

qingqing01 · 2018-01-12T06:42:29Z

paddle/operators/prior_box_op.cc

Need to explain 2, layer_height, layer_width, num_priors, 4. Need more explanation for the layout.

qingqing01 · 2018-01-12T06:47:06Z

paddle/operators/prior_box_op.cc

Why need attrs of img_w and img_h?

qingqing01 · 2018-01-12T06:50:15Z

paddle/operators/prior_box_op.cc

Why need .SetDefault({}) ?

qingqing01 · 2018-01-12T06:55:10Z

paddle/operators/prior_box_op.cc

I think we need to give more doc for this operator, how to generator box? how to calculate the number of prior boxes, and so on?

The TF doc is:

https://github.com/tensorflow/models/blob/master/research/object_detection/core/anchor_generator.py

I cannot see any information in this comments.

qingqing01 · 2018-01-12T06:56:48Z

paddle/operators/prior_box_op.cu

If not implement the GPU, do not need to register GPU kernel.

qingqing01 · 2018-01-12T07:12:41Z

paddle/operators/prior_box_op.h

int64_t, since the type in input->dims() is int64_t

qingqing01 · 2018-01-12T07:16:52Z

paddle/operators/prior_box_op.h

Now support multi-device, there is no need to copy tensor from GPU to CPU.

qingqing01 · 2018-01-12T07:19:15Z

paddle/operators/prior_box_op.h

这里的clip看下能不能调用Eigen的函数，而不是4层for循环。

可以参考 https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/compare_op.h#L72 ，用 platform::Transform 也行。

qingqing01 · 2018-01-12T07:19:50Z

paddle/operators/prior_box_op.h

同上，这种赋值，看看下能不能调用Eigen的函数，而不是4层for循环。

这里用broadcast即可~

qingqing01 · 2018-01-12T07:20:36Z

python/paddle/v2/fluid/tests/test_prior_box_op.py

Python单测需要改进，尽量向量操作，不用这么多层的for循环。

… prior_box

wanghaox

update code

wanghaox · 2018-01-12T07:41:19Z

paddle/operators/prior_box_op.cc

wanghaox · 2018-01-12T07:41:24Z

paddle/operators/prior_box_op.cc

wanghaox · 2018-01-12T07:43:06Z

paddle/operators/prior_box_op.cc

wanghaox · 2018-01-12T07:44:56Z

paddle/operators/prior_box_op.cc

wanghaox · 2018-01-12T07:45:20Z

paddle/operators/prior_box_op.cc

wanghaox · 2018-01-12T16:38:46Z

paddle/operators/prior_box_op.cc

wanghaox · 2018-01-12T16:39:17Z

paddle/operators/prior_box_op.h

wanghaox · 2018-01-12T16:39:27Z

paddle/operators/prior_box_op.h

wanghaox · 2018-01-12T16:39:37Z

python/paddle/v2/fluid/tests/test_prior_box_op.py

wanghaox · 2018-01-13T01:08:26Z

paddle/operators/prior_box_op.h

+inline void expand_aspect_ratios(const std::vector<float> input_aspect_ratior,
+                                 bool flip,
+                                 std::vector<float>& output_aspect_ratior) {
+  constexpr float eps = 1e-6;


qingqing01 · 2018-01-15T11:59:08Z

paddle/operators/prior_box_op.cc

+    AddAttr<std::vector<float>>(
+        "aspect_ratios", "(vector<float>) ",
+        "List of aspect ratios of generated prior boxes.")
+        .SetDefault({});


Remove .SetDefault({}) too.

qingqing01 · 2018-01-15T12:01:35Z

paddle/operators/prior_box_op.cc

+      }
+    }
+
+    if (variances.size() > 1) {


It seems line 68 should be removed.

qingqing01 · 2018-01-15T12:05:54Z

paddle/operators/prior_box_op.h

+      for (int w = 0; w < layer_width; ++w) {
+        float center_x = (w + offset) * step_width;
+        float center_y = (h + offset) * step_height;
+        float box_width, box_height;


template <typename Place, typename T>

If T is double, here also should be double.

qingqing01 · 2018-01-15T12:17:02Z

python/paddle/v2/fluid/tests/test_prior_box_op.py

+                        center_x + box_width / 2.) / self.image_w
+                    # ymax
+                    out_boxes[h, w, idx, 3] = (
+                        center_y + box_height / 2.) / self.image_h


这块代码可以简化下，

c_x = (w + self.offset) * self.step_w c_y = (h + self.offset) * self.step_h # ... c_w = c_h = min_size/2. out_boxes[h, w, idx, :] = [(c_x - c_w)/self.image_w, (c_y - c_h)/self.image_h, ..., ...] # ...

下面两处计算相同，可以使代码更短一些。

pkuyym · 2018-01-15T12:21:31Z

paddle/operators/prior_box_op.cc

+
+  void InferShape(framework::InferShapeContext* ctx) const override {
+    PADDLE_ENFORCE(ctx->HasInput("Input"),
+                   "Input(X) of PriorBoxOp should not be null.");


Not Input(X), should be Input(Input).

pkuyym · 2018-01-15T12:21:55Z

paddle/operators/prior_box_op.cc

+    PADDLE_ENFORCE(ctx->HasInput("Input"),
+                   "Input(X) of PriorBoxOp should not be null.");
+    PADDLE_ENFORCE(ctx->HasInput("Image"),
+                   "Input(Offset) of PriorBoxOp should not be null.");


Input(Offset) --> Input(Image)

pkuyym · 2018-01-15T12:22:35Z

paddle/operators/prior_box_op.cc

+
+    auto image_dims = ctx->GetInputDim("Image");
+    auto input_dims = ctx->GetInputDim("Input");
+    PADDLE_ENFORCE(image_dims.size() == 4, "The format of image is NCHW.");


I think The layout of data is NCHW is better.

pkuyym · 2018-01-15T12:23:41Z

paddle/operators/prior_box_op.cc

+    bool flip = ctx->Attrs().Get<bool>("flip");
+
+    PADDLE_ENFORCE_GT(min_sizes.size(), 0,
+                      "Size of min_size must be at least 1.");


min_size --> min_sizes

pkuyym · 2018-01-15T12:25:07Z

paddle/operators/prior_box_op.cc

+    int num_priors = aspect_ratios_vec.size() * min_sizes.size();
+    if (max_sizes.size() > 0) {
+      PADDLE_ENFORCE_EQ(max_sizes.size(), min_sizes.size(),
+                        "The length of min_size and max_size must be equal.");


length --> number

pkuyym · 2018-01-15T12:25:49Z

paddle/operators/prior_box_op.cc

+
+    auto min_sizes = ctx->Attrs().Get<std::vector<int>>("min_sizes");
+    auto max_sizes = ctx->Attrs().Get<std::vector<int>>("max_sizes");
+    auto variances = ctx->Attrs().Get<std::vector<float>>("variances");


Should be variances optional?

here variances are needed for output "Variances"

… prior_box

pkuyym · 2018-01-23T04:00:32Z

paddle/operators/prior_box_op.h

+            boxes->data<T>(), clip_func);
+    }
+
+    Eigen::Tensor<T, 2, Eigen::RowMajor> var_et(1, variances.size());


I think it is more efficiency to use framework::Tensor whose memory is allocated from internal pool. @qingqing01 Please help to confirm.

Yeah, should use Fluid's memory (or Tenosr) to allocate auxiliary workspace.

qingqing01 · 2018-01-23T06:16:55Z

paddle/operators/prior_box_op.cc

+    AddOutput("Boxes",
+              "(Tensor, default Tensor<float>), the output prior boxes of "
+              "PriorBoxOp. The layout is [layer_height, layer_width, "
+              "num_priors, 4]. layer_height is the height of input, "


layer_height -> H,
layer_width -> W

same as below.

qingqing01 · 2018-01-23T06:20:48Z

paddle/operators/prior_box_op.h

+    auto img_height = image->dims()[2];
+
+    auto layer_width = input->dims()[3];
+    auto layer_height = input->dims()[2];


layer_width -> feature_width or width.

Same as layer_height

… prior_box

implement of prior box operator for ssd

ee0113a

wanghaox requested review from chengduoZH, kexinzhao, pkuyym and qingqing01 and removed request for kexinzhao December 1, 2017 07:32

pkuyym previously requested changes Dec 6, 2017

View reviewed changes

fix some issues

7297e6f

wanghaox commented Dec 13, 2017

View reviewed changes

sweetsky0901 reviewed Jan 3, 2018

View reviewed changes

change output shape to [2, layer_height, layer_width, num_priors, 4]

99a6c5d

qingqing01 requested changes Jan 12, 2018

View reviewed changes

wanghaox added 2 commits January 13, 2018 08:49

update code

1ba3d29

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

8ab611d

… prior_box

wanghaox commented Jan 13, 2018

View reviewed changes

qingqing01 reviewed Jan 15, 2018

View reviewed changes

pkuyym requested changes Jan 15, 2018

View reviewed changes

qingqing01 mentioned this pull request Jan 17, 2018

The TODO lists for MobileNet-SSD model. #7488

Closed

25 tasks

wanghaox added 5 commits January 22, 2018 14:05

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

f020f4b

… prior_box

update code

142f632

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

f7c0ad9

… prior_box

update code

0e16503

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

d662e85

… prior_box

pkuyym reviewed Jan 23, 2018

View reviewed changes

qingqing01 reviewed Jan 23, 2018

View reviewed changes

wanghaox added 2 commits January 23, 2018 14:34

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

534cf74

… prior_box

update code

ca2e96f

qingqing01 approved these changes Jan 23, 2018

View reviewed changes

pkuyym approved these changes Jan 23, 2018

View reviewed changes

wanghaox merged commit 81be9ce into PaddlePaddle:develop Jan 23, 2018

wanghaox deleted the prior_box branch January 23, 2018 08:57

Comments

Conversation

wanghaox commented Dec 1, 2017

Uh oh!

pkuyym 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!

Choose a reason for hiding this comment

Uh oh!

wanghaox 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!

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!