Roi pool operator #5831

wanghaox · 2017-11-22T08:47:45Z

resolve #5788

Add CPU forward and backward kernel code
Add GPU forward and backward kernel code
Add unitest

update from origin

… develop

wangkuiyi · 2017-11-22T21:00:38Z

paddle/operators/roi_pool_op.cu

+
+#define FLT_MAX __FLT_MAX__
+
+constexpr int PADDLE_OPERATORS_ROIPOOL_CUDA_NUM_THREADS = 512;


According to https://google.github.io/styleguide/cppguide.html#Constant_Names, constants should be named as

constexpr int kPaddleOperatorsROIPoolNumCUDAThreads = 512;

Also, I vaguely remember (but not sure) that we can declare this variable static so to limit its usage within this file, and we can have a much shorter name for it:

static constexpr int kNumCUDAThreads = 512;

wangkuiyi · 2017-11-22T21:06:08Z

paddle/operators/roi_pool_op.cu

+namespace paddle {
+namespace operators {
+
+#define FLT_MAX __FLT_MAX__


The use of __FLT_MAX__ is not a good idea because it is a GCC-specific predefined macro. If we are using another compiler other than GCC, it is probable that we don't have __FLT_MAX__ defined.

It seems to me that the solution could be using standard C library

#include <float.h> printf("%f", FLT_MAX);

or the standard C++ library

#include <limits> printf("%f", numeric_limits<float>::max());

This problem doesn't block the merge of this PR. I created an issue reminding us to fix it.

wangkuiyi · 2017-11-22T21:12:09Z

paddle/operators/roi_pool_op.cu

+constexpr int PADDLE_OPERATORS_ROIPOOL_CUDA_NUM_THREADS = 512;
+constexpr int PADDLE_OPERATORS_ROIPOOL_MAXIMUM_NUM_BLOCKS = 4096;
+
+inline int PADDLE_OPERATORS_ROIPOOL_GET_BLOCKS(const int N) {


I understand that you might want to mark this function returning a constant value, but according to the Google C++ Style Guide, we should name functions something like

static inline int NumBlocks(const int N) {

wangkuiyi · 2017-11-22T21:14:18Z

paddle/operators/roi_pool_op.cc

+namespace paddle {
+namespace operators {
+
+class RoiPoolOp : public framework::OperatorWithKernel {


I understand that in many places of our codebase, we haven't strictly followed the English writing rule that acronyms must be capitalized, but let us start doing so from here by changing Roi into ROI. Thanks!

wangkuiyi · 2017-11-22T21:15:46Z

paddle/operators/roi_pool_op.cu

+      int c = (index / pooled_width / pooled_height) % channels;
+      int n = index / pooled_width / pooled_height / channels;
+
+      const int64_t* offset_input_rois = input_rois + n * 5;


How about add a comment about the magic number 5, or define it a constexpr value?

wangkuiyi · 2017-11-22T21:20:14Z

Is this ROIPool operator being added into CMakeLists.txt? @wanghaox

guoshengCS · 2017-11-24T02:53:15Z

paddle/operators/roi_pool_op.h

+                      "The spatial scale must greater than 0");
+
+    auto in_dims = in->dims();
+    int batch_size = in_dims[0];


Might it be better to add dim ENFORCE of input before or in inferShape since the usage of in_dim.

done, added at ROIPoolOp::InferShape()

guoshengCS · 2017-11-24T02:54:44Z

paddle/operators/roi_pool_op.h

+    int channels = in_dims[1];
+    int height = in_dims[2];
+    int width = in_dims[3];
+    int rois_num = rois->dims()[0];


Similar with in_dims, might it be better to add dim ENFORCE before.

done, added at ROIPoolOp::InferShape()

guoshengCS · 2017-11-24T03:08:04Z

paddle/operators/roi_pool_op.cc

+             "(Tensor), "
+             "ROIs (Regions of Interest) to pool over. "
+             "Should be a 2-D tensor of shape (num_rois, 5)"
+             "given as [[batch_id, x1, y1, x2, y2], …].");


Might it be better to declare what each of [batch_id, x1, y1, x2, y2] represents for. And some spelling like Should should be correct.

guoshengCS · 2017-11-24T03:09:48Z

paddle/operators/roi_pool_op.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X",
+             "(Tensor), "
+             "the input of ROIPoolOp.");


Addition of the shape and meaning(like feature maps from conv) of X might be better.

guoshengCS · 2017-11-24T03:18:40Z

paddle/operators/roi_pool_op.h

+    out_dims[1] = channels;
+    out_dims[2] = pooled_height;
+    out_dims[3] = pooled_width;
+    out->Resize(out_dims);


Might it be better to set shape in inferShape rather than shape resizing in here, but I am not sure whether actual shapes are necessary for inferShape.

done, added at ROIPoolOp::InferShape()

guoshengCS · 2017-11-24T06:35:47Z

paddle/operators/roi_pool_op.cc

+             "given as [[batch_id, x1, y1, x2, y2], …].");
+    AddOutput("Out",
+              "(Tensor), "
+             "ROI pooled output 4-D tensor of shape "


Maybe "The output of ROIPoolOp is a 4-D tensor with shape" is better.

guoshengCS · 2017-11-24T06:46:53Z

paddle/operators/roi_pool_op.cu

+
+    auto in_dims = in->dims();
+    auto in_stride = framework::stride(in_dims);
+    int channels = in_dims[1];


Similar to the CPU kernel, might it be better to add dim ENFORCE before.

done, added at ROIPoolOp::InferShape()

guoshengCS · 2017-11-24T07:16:09Z

paddle/operators/roi_pool_op.cu

+      out->mutable_data<T>(ctx.GetPlace()),
+      argmax->mutable_data<int64_t>(ctx.GetPlace()));
+
+      return;


How about deleting the return in here.

guoshengCS · 2017-11-24T07:16:55Z

paddle/operators/roi_pool_op.cu

+          pooled_width,
+          x_grad->mutable_data<T>(ctx.GetPlace()));
+        }
+      return;


How about deleting the return in here.

guoshengCS · 2017-11-24T07:17:17Z

paddle/operators/roi_pool_op.cu

+    auto spatial_scale = ctx.Attr<float>("spatial_scale");
+
+    int rois_num = rois->dims()[0];
+    int channels = in->dims()[1];


Similar to the CPU kernel, might it be better to add dim ENFORCE before.

done, added at ROIPoolOp::InferShape()

wanghaox

update code

wanghaox · 2017-11-24T08:18:22Z

paddle/operators/roi_pool_op.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X",
+             "(Tensor), "
+             "the input of ROIPoolOp.");


wanghaox · 2017-11-24T08:30:14Z

paddle/operators/roi_pool_op.cc

+             "(Tensor), "
+             "ROIs (Regions of Interest) to pool over. "
+             "Should be a 2-D tensor of shape (num_rois, 5)"
+             "given as [[batch_id, x1, y1, x2, y2], …].");


wanghaox · 2017-11-24T08:30:41Z

paddle/operators/roi_pool_op.cc

+             "given as [[batch_id, x1, y1, x2, y2], …].");
+    AddOutput("Out",
+              "(Tensor), "
+             "ROI pooled output 4-D tensor of shape "


wanghaox · 2017-11-24T08:34:05Z

paddle/operators/roi_pool_op.cu

+      out->mutable_data<T>(ctx.GetPlace()),
+      argmax->mutable_data<int64_t>(ctx.GetPlace()));
+
+      return;


wanghaox · 2017-11-24T08:35:47Z

paddle/operators/roi_pool_op.cu

+          pooled_width,
+          x_grad->mutable_data<T>(ctx.GetPlace()));
+        }
+      return;


wanghaox · 2017-11-24T08:52:55Z

paddle/operators/roi_pool_op.h

+    int channels = in_dims[1];
+    int height = in_dims[2];
+    int width = in_dims[3];
+    int rois_num = rois->dims()[0];


done, added at ROIPoolOp::InferShape()

wanghaox · 2017-11-24T08:55:10Z

paddle/operators/roi_pool_op.cu

+    auto spatial_scale = ctx.Attr<float>("spatial_scale");
+
+    int rois_num = rois->dims()[0];
+    int channels = in->dims()[1];


done, added at ROIPoolOp::InferShape()

wanghaox · 2017-11-24T08:55:20Z

paddle/operators/roi_pool_op.cu

+
+    auto in_dims = in->dims();
+    auto in_stride = framework::stride(in_dims);
+    int channels = in_dims[1];


done, added at ROIPoolOp::InferShape()

wanghaox · 2017-11-24T08:55:54Z

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

+                            for w in range(wstart, wend):
+                                if x_i[c, h, w] > out_data[i, c, ph, pw]:
+                                    out_data[i, c, ph, pw] = x_i[c, h, w]
+                                    argmax_data[i, c, ph, pw] = h * \


python needs the '\'

wanghaox · 2017-11-24T09:22:58Z

paddle/operators/roi_pool_op.h

+      size_t pool_channel_offset = pooled_height * pooled_width;
+      const int64_t* argmax_data = argmax->data<int64_t>();
+
+      for (size_t n = 0; n < rois_num; ++n) {


qingqing01 · 2017-11-24T10:19:43Z

paddle/operators/roi_pool_op.cc

+    ops::CPUROIPoolOpKernel<paddle::platform::CPUPlace, float>);
+REGISTER_OP_CPU_KERNEL(
+    roi_pool_grad,
+    ops::CPUROIPoolGradOpKernel<paddle::platform::CPUPlace, float>);


Need to register double type. And please check whether the slice_sequence_op registered double type, thanks.

done, slice_sequence_op will be fixed in another commit.

qingqing01 · 2017-11-24T10:21:30Z

paddle/operators/roi_pool_op.h

+#pragma once
+#include "paddle/framework/op_registry.h"
+#include "paddle/operators/math/math_function.h"
+#include "paddle/operators/strided_memcpy.h"


This header file is not used?

qingqing01 · 2017-11-24T10:22:42Z

paddle/operators/roi_pool_op.h

+
+using Tensor = framework::Tensor;
+using LoDTensor = framework::LoDTensor;
+using LoD = framework::LoD;


see comments: #5826 (comment)

qingqing01 · 2017-11-24T10:28:33Z

paddle/operators/roi_pool_op.h

+            // Define an empty pooling region to be zero
+            bool is_empty = (hend <= hstart) || (wend <= wstart);
+            output_data[pool_index] =
+                is_empty ? 0 : -std::numeric_limits<float>::max();


-std::numeric_limits<T>

qingqing01 · 2017-11-24T10:29:32Z

paddle/operators/roi_pool_op.h

+    int64_t* argmax_data = argmax->mutable_data<int64_t>(ctx.GetPlace());
+
+    math::SetConstant<Place, T> set_zero;
+    set_zero(ctx.device_context(), out, static_cast<T>(0));


There is no need to set out zero here.

qingqing01 · 2017-11-24T10:37:12Z

paddle/operators/roi_pool_op.cc

+                   "The format of input tensor is NCHW.");
+    PADDLE_ENFORCE(rois_dims.size() == 2,
+                   "ROIs should be a 2-D tensor of shape (num_rois, 5)"
+                   "given as [[batch_id, x1, y1, x2, y2], …].");


Also needs to check rois_dims[1] == kROISize

qingqing01 · 2017-11-24T10:38:08Z

paddle/operators/roi_pool_op.cu

+    set_zero(ctx.device_context(), out, static_cast<T>(0));
+    argmax->mutable_data<int64_t>(ctx.GetPlace());
+    math::SetConstant<Place, int64_t> set_init;
+    set_init(ctx.device_context(), argmax, static_cast<int64_t>(-1));


There is no need to set out zero and set argmax -1 here. For GPU, will launch two kernels twice here. Please remove these and handle them in GPUROIPoolForward correctly.

qingqing01 · 2017-11-24T10:55:38Z

paddle/operators/roi_pool_op.cu

+    if (x_grad) {
+      x_grad->mutable_data<T>(ctx.GetPlace());
+      math::SetConstant<Place, T> set_zero;
+      set_zero(ctx.device_context(), x_grad, static_cast<T>(0));


Same as above, there is no need to set zero here.

checked, bp needs to set zero.

qingqing01 · 2017-11-24T10:55:50Z

paddle/operators/roi_pool_op.cu

+    ops::GPUROIPoolOpKernel<paddle::platform::GPUPlace, float>);
+REGISTER_OP_GPU_KERNEL(
+    roi_pool_grad,
+    ops::GPUROIPoolGradOpKernel<paddle::platform::GPUPlace, float>);


register double type.

qingqing01 · 2017-11-24T11:03:03Z

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

+                        wstart = min(max(wstart + roi_start_w, 0), self.width)
+                        wend = min(max(wend + roi_start_w, 0), self.width)
+
+                        out_data[i, c, ph, pw] = 0


As @guoshengCS said, why the code here is not consistant with C++:

// Define an empty pooling region to be zero bool is_empty = (hend <= hstart) || (wend <= wstart); output_data[pool_index] = is_empty ? 0 : -std::numeric_limits<float>::max();

wanghaox

update code

wanghaox · 2017-11-24T11:12:03Z

paddle/operators/roi_pool_op.cc

+                   "The format of input tensor is NCHW.");
+    PADDLE_ENFORCE(rois_dims.size() == 2,
+                   "ROIs should be a 2-D tensor of shape (num_rois, 5)"
+                   "given as [[batch_id, x1, y1, x2, y2], …].");


wanghaox · 2017-11-24T11:13:18Z

paddle/operators/roi_pool_op.cc

+    ops::CPUROIPoolOpKernel<paddle::platform::CPUPlace, float>);
+REGISTER_OP_CPU_KERNEL(
+    roi_pool_grad,
+    ops::CPUROIPoolGradOpKernel<paddle::platform::CPUPlace, float>);


done, slice_sequence_op will be fixed in another commit.

wanghaox · 2017-11-24T11:14:38Z

paddle/operators/roi_pool_op.cu

+static inline int NumBlocks(const int N) {
+  return std::min((N + kNumCUDAThreads - 1) / kNumCUDAThreads,
+                  kNumMaxinumNumBlocks);
+  }


wanghaox · 2017-11-24T11:16:31Z

paddle/operators/roi_pool_op.cu

+    ops::GPUROIPoolOpKernel<paddle::platform::GPUPlace, float>);
+REGISTER_OP_GPU_KERNEL(
+    roi_pool_grad,
+    ops::GPUROIPoolGradOpKernel<paddle::platform::GPUPlace, float>);


wanghaox · 2017-11-24T11:16:55Z

paddle/operators/roi_pool_op.h

+#pragma once
+#include "paddle/framework/op_registry.h"
+#include "paddle/operators/math/math_function.h"
+#include "paddle/operators/strided_memcpy.h"


wanghaox · 2017-11-24T12:03:02Z

paddle/operators/roi_pool_op.cu

+    set_zero(ctx.device_context(), out, static_cast<T>(0));
+    argmax->mutable_data<int64_t>(ctx.GetPlace());
+    math::SetConstant<Place, int64_t> set_init;
+    set_init(ctx.device_context(), argmax, static_cast<int64_t>(-1));


wanghaox · 2017-11-24T12:03:42Z

paddle/operators/roi_pool_op.cu

+    if (x_grad) {
+      x_grad->mutable_data<T>(ctx.GetPlace());
+      math::SetConstant<Place, T> set_zero;
+      set_zero(ctx.device_context(), x_grad, static_cast<T>(0));


checked, bp needs to set zero.

wanghaox · 2017-11-24T12:04:08Z

paddle/operators/roi_pool_op.h

+
+using Tensor = framework::Tensor;
+using LoDTensor = framework::LoDTensor;
+using LoD = framework::LoD;


wanghaox · 2017-11-24T12:04:22Z

paddle/operators/roi_pool_op.h

+    int64_t* argmax_data = argmax->mutable_data<int64_t>(ctx.GetPlace());
+
+    math::SetConstant<Place, T> set_zero;
+    set_zero(ctx.device_context(), out, static_cast<T>(0));


wanghaox · 2017-11-24T12:05:06Z

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

+                        wstart = min(max(wstart + roi_start_w, 0), self.width)
+                        wend = min(max(wend + roi_start_w, 0), self.width)
+
+                        out_data[i, c, ph, pw] = 0


qingqing01

LGTM.

wanghaox and others added 4 commits November 13, 2017 21:56

Merge pull request #1 from PaddlePaddle/develop

ca988bd

update from origin

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

9216da3

… develop

add roi pool operator

7960928

add roi operator unittest

36dd770

wanghaox added the OpPorting label Nov 22, 2017

wanghaox force-pushed the roi_pool branch 2 times, most recently from 25f84c7 to e9d8c71 Compare November 22, 2017 13:21

wangkuiyi previously approved these changes Nov 22, 2017

View reviewed changes

wanghaox dismissed wangkuiyi’s stale review via 11e5a06 November 23, 2017 02:39

wanghaox force-pushed the roi_pool branch 3 times, most recently from a9a3f15 to 1aba956 Compare November 23, 2017 02:49

qingqing01 requested a review from guoshengCS November 23, 2017 06:49

guoshengCS reviewed Nov 24, 2017

View reviewed changes

fix some code issues

ef90559

wanghaox force-pushed the roi_pool branch from 1aba956 to ef90559 Compare November 24, 2017 09:29

wanghaox commented Nov 24, 2017

View reviewed changes

qingqing01 reviewed Nov 24, 2017

View reviewed changes

fix some issues

cf5b598

wanghaox commented Nov 24, 2017

View reviewed changes

qingqing01 approved these changes Nov 24, 2017

View reviewed changes

wanghaox merged commit 0690cca into PaddlePaddle:develop Nov 24, 2017

wanghaox deleted the roi_pool branch November 24, 2017 13:22


		#define FLT_MAX __FLT_MAX__

		constexpr int PADDLE_OPERATORS_ROIPOOL_CUDA_NUM_THREADS = 512;

Roi pool operator #5831

Roi pool operator #5831

Conversation

wanghaox commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanghaox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanghaox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qingqing01 left a comment

Choose a reason for hiding this comment