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

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

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

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

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

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

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

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

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

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

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

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

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

How about deleting the return in here.

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

paddle/operators/roi_pool_op.cu

How about deleting the return in here.

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

paddle/operators/roi_pool_op.cu

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

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

paddle/operators/roi_pool_op.cc

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

paddle/operators/roi_pool_op.cc

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

paddle/operators/roi_pool_op.cu

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

paddle/operators/roi_pool_op.cu

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

paddle/operators/roi_pool_op.h

done, added at ROIPoolOp::InferShape()

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

paddle/operators/roi_pool_op.cu

done, added at ROIPoolOp::InferShape()

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

paddle/operators/roi_pool_op.cu

done, added at ROIPoolOp::InferShape()

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

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

python needs the '\'

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

paddle/operators/roi_pool_op.h

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

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

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

wangkuiyi previously approved these changes Nov 22, 2017

View reviewed changes

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

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

Roi pool operator #5831

Roi pool operator #5831

Uh oh!

Conversation

wanghaox commented Nov 22, 2017

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!

wangkuiyi commented Nov 22, 2017

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!

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