Convolution operator #4042

hedaoyuan · 2017-09-12T10:10:35Z

No description provided.

QiJune · 2017-09-12T12:38:04Z

python/paddle/v2/framework/tests/test_conv2d_op.py

+from paddle.v2.framework.op import Operator
+
+
+class TestConv2dOp(unittest.TestCase):


The operator python test framework has been refactored, please merge develop branch and change the unit test accordingly.

chengduoZH · 2017-09-13T12:17:49Z

paddle/operators/gemm_conv_op.h

+using Tensor = framework::Tensor;
+
+template <typename Place, typename T>
+class GemmConvKernel : public framework::OpKernel {


We'll write the 3D convolution later. Should we distinguish the names? GemmConvKernel->GemmConv2DKernel, GemmConvGradKernel -> GemmConv2dGradKernel, gemm_conv_op.h->gemm_conv2d_op.h, conv_op.cu->conv2d_op.cu

chengduoZH · 2017-09-13T12:25:05Z

paddle/operators/conv_op.cc

+namespace paddle {
+namespace operators {
+
+int outputSize(int input_size, int filter_size, int padding, int stride) {


This function is also used in conv3d, pooling2d, pooling3d. Should it be written in one place?

I think this can be fixed in the next PR. At present, it is not sure where to put this function is better.

typhoonzero · 2017-09-14T13:09:21Z

paddle/operators/conv_op.cc

+REGISTER_OP(conv2d, ops::Conv2DOp, ops::Conv2DOpMaker, conv2d_grad,
+            ops::Conv2DOpGrad);
+
+REGISTER_OP_CPU_KERNEL(conv2d,


The current build system requires the filename matches the registered operator name. Maybe rename them both to conv or conv2d.

typhoonzero · 2017-09-14T13:10:57Z

paddle/operators/conv_op.cc

+  }
+};
+
+class Conv2DOpMaker : public framework::OpProtoAndCheckerMaker {


Can we put Conv2DOpMaker and CPU implementation in a base class like ConvBase? so that CUDA gemm implementation and cudnn implementation can reuse the code.

I think we do not need to write a Conv2DOpMaker for CudnnConv.
CudnnConv also can use the Conv2DOpMaker class.

chengduoZH · 2017-09-15T04:19:07Z

paddle/operators/gemm_conv_op.h

+    auto t1 = framework::EigenVector<T>::Flatten(filter_grad);
+    t1.device(context.GetEigenDevice<Place>()) = t1.constant(static_cast<T>(0));
+    auto t2 = framework::EigenVector<T>::Flatten(*input_grad);
+    t2.device(context.GetEigenDevice<Place>()) = t2.constant(static_cast<T>(0));


Shouldn't the gradient be cleared here? The weights entered between different Op may be shared.

The weights entered between different Op may be shared.

If the weights are shared, the framework is responsible for merge the two parts of the gradients.
The gradient tensor in other op is also cleared.
https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/lookup_table_op.h#L60

qingqing01 · 2017-09-14T13:35:59Z

paddle/operators/conv_op.cc

+    int input_channels = in->dims()[1];
+    int output_channels = filter->dims()[0];
+
+    PADDLE_ENFORCE_EQ(in->dims().size(), 4, "Conv2DOp intput should be 4-D.");


intput -> input

qingqing01 · 2017-09-14T13:36:18Z

paddle/operators/conv_op.cc

+  void InferShape(const framework::InferShapeContext &ctx) const override {
+    auto in = ctx.Input<Tensor>("Input");
+    auto filter = ctx.Input<Tensor>("Filter");
+    auto out = ctx.Output<Tensor>("Output");


Output<Tensor> -> Output<LoDTensor>

qingqing01 · 2017-09-15T08:05:16Z

paddle/operators/conv_op.cc

+)DOC");
+    AddAttr<std::vector<int>>("strides", "strides of convolution operator.");
+    AddAttr<std::vector<int>>("paddings", "paddings of convolution operator.");
+    AddAttr<int>(


Put the attr before doc.

qingqing01 · 2017-09-15T08:05:37Z

paddle/operators/conv_op.cc

+    auto in = ctx.Input<Tensor>("Input");
+    auto filter = ctx.Input<Tensor>("Filter");
+    auto d_in = ctx.Output<Tensor>(framework::GradVarName("Input"));
+    auto d_filter = ctx.Output<Tensor>(framework::GradVarName("Filter"));


Output< framework::LoDTensor>

qingqing01 · 2017-09-18T09:14:44Z

paddle/operators/conv2d_op.cc

+        "when group=2, the first half of the filters are only connected to the "
+        "first half of the input channels, and the second half only connected "
+        "to the second half.")
+        .SetDefault(1);


Put AddAttr before AddComment(R"DOC )DOC").

qingqing01 · 2017-09-18T09:47:23Z

paddle/operators/gemm_conv2d_op.h

+        context.Input<Tensor>(framework::GradVarName("Output"));
+    Tensor* input_grad =
+        context.Output<Tensor>(framework::GradVarName("Input"));
+    Tensor* filter_grad_ =


filter_grad_ -> filter_grad

这个不行，后面定义了一个filter_grad变量。

qingqing01 · 2017-09-18T09:51:20Z

paddle/operators/gemm_conv2d_op.h

+    auto t1 = framework::EigenVector<T>::Flatten(filter_grad);
+    t1.device(context.GetEigenDevice<Place>()) = t1.constant(static_cast<T>(0));
+    auto t2 = framework::EigenVector<T>::Flatten(*input_grad);
+    t2.device(context.GetEigenDevice<Place>()) = t2.constant(static_cast<T>(0));


Why need to zero memory for input_grad?

I remember talking to you about this problem, and your suggestion is that operator needs = operation instead of += operation.

qingqing01 · 2017-09-18T10:12:10Z

paddle/operators/gemm_conv2d_op.h

+        Tensor filter_grad_slice =
+            filter_grad.Slice<T>(g * out_step, (g + 1) * out_step);
+        math::matmul<Place, T>(out_grad_slice, false, col_matrix, true, T(1.0),
+                               &filter_grad_slice, T(1.0), device_context);


2. `gradient w.r.t. weight` 和 `gradient w.r.t. input data` 的计算，是否需要提出来两个函数？ 3. 需要考虑 `gradient w.r.t. weight` 或者 `gradient w.r.t. input data` 不计算的情况，类似 mul_op的情况： https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/mul_op.h#L76

Backward对于gradient w.r.t. input data的计算和通常的实现(Paddle老的，Caffe)不同，是否需要说明下？

这个不同指的是什么？

gradient w.r.t. weight 和 gradient w.r.t. input data 的计算，是否需要提出来两个函数？

目前来看，提出来两个函数好像并没有什么用，caffe2里面也并没有去提出两个函数。

这个不同指的是什么？

上面看comment错了，可以忽略吧 :)

qingqing01 · 2017-09-18T10:14:56Z

python/paddle/v2/framework/tests/test_conv2d_op.py

+                                                frowid][fcolid]
+                                        output_value += input_value * filter_value
+                            output[batchid][outchannelid][rowid][
+                                colid] = output_value


conv的计算可否用： https://docs.scipy.org/doc/numpy-1.13.0/reference/generated/numpy.convolve.html ？

可以试一下，不过这个好像不支持group。

不能用np.convolve
'Returns the discrete, linear convolution of two one-dimensional sequences.'

qingqing01 · 2017-09-18T10:16:31Z

python/paddle/v2/framework/tests/test_conv2d_op.py

+        self.check_output()
+
+    def test_check_grad(self):
+        self.check_grad(set(['Input', 'Filter']), 'Output')


需要类似mul_op一样的check，检车Input或者Filter不需要Grad的情况： https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/framework/tests/test_mul_op.py#L49

qingqing01 · 2017-09-18T10:18:19Z

另外， convolution operator是否需要考虑加bias?

Xreki · 2017-09-18T14:18:17Z

Paddle的Layer里面，应该都是支持多输入和activation的，Op需要支持吗？

hedaoyuan · 2017-09-19T03:21:18Z

暂时不考虑在GemmConv2DKernel里面支持bias, 多输入，activation等。跟这个GemmConv2DKernel一样的还会有一个CudnnConv2DKernel也不会支持bias，多输入，activation这些。后续可以考虑一个更上层的OpConvLayer，来支持bias，多输入这些。

typhoonzero

LGTM++

chengduoZH · 2017-09-25T12:44:52Z

fix #3691

hedaoyuan added 3 commits September 11, 2017 21:25

Convolution op and forward calculation.

c9d8cb4

Add backward of convolution.

40fe0a8

Merge branch 'develop' of https://github.com/baidu/Paddle into conv_op

3705de6

QiJune reviewed Sep 12, 2017

View reviewed changes

hedaoyuan added 7 commits September 12, 2017 20:49

Fix test_conv2d_op.py.

c671189

Refine test_conv2d_op.py

a7c1872

Refine the GemmConvKernel.

67db9d3

Refine the GemmConvGradKernel.

db33ff1

Fix Tensor::Slice with dims[0] == 1.

5860150

Refine gemm convolution kernel.

8219f20

Merge branch 'develop' of https://github.com/baidu/Paddle into conv_op

14ae805

hedaoyuan requested review from qingqing01 and chengduoZH September 13, 2017 03:06

qingqing01 added the OpPorting label Sep 13, 2017

qingqing01 requested a review from Xreki September 13, 2017 03:23

hedaoyuan added 4 commits September 13, 2017 14:15

Add groups in convolution operator.

fb46345

Add groups in convolution GemmConvGradKernel.

2340ced

Bug fix.

1dd639e

Add groups test.

b4ba35c

chengduoZH reviewed Sep 13, 2017

View reviewed changes

Fix the doc.

656f775

typhoonzero reviewed Sep 14, 2017

View reviewed changes

chengduoZH reviewed Sep 15, 2017

View reviewed changes

hedaoyuan added 4 commits September 17, 2017 23:50

Merge branch 'develop' of https://github.com/baidu/Paddle into conv_op

7bf1e76

Follow comments.

09c65b6

Some bug fix.

91afa0d

Add test with groups=1.

5a4138b

qingqing01 reviewed Sep 18, 2017

View reviewed changes

Follow comments fix conv2d_op.cc

64b0b75

hedaoyuan added 2 commits September 18, 2017 23:48

Support input_grad = null or filter_grad = null.

f3669ca

Refine the GemmConvGrad2DKernel.

6c0129a

typhoonzero mentioned this pull request Sep 19, 2017

Cudnn conv op #4195

Merged

3 tasks

typhoonzero approved these changes Sep 20, 2017

View reviewed changes

hedaoyuan merged commit 7a891a3 into PaddlePaddle:develop Sep 21, 2017

hedaoyuan mentioned this pull request Sep 21, 2017

compiler failed due to gemm_conv2d_op.h #4286

Closed

		from paddle.v2.framework.op import Operator


		class TestConv2dOp(unittest.TestCase):

Convolution operator #4042

Convolution operator #4042

Conversation

hedaoyuan commented Sep 12, 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

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 Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qingqing01 Sep 18, 2017 • edited Loading

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 commented Sep 18, 2017

Xreki commented Sep 18, 2017

hedaoyuan commented Sep 19, 2017

typhoonzero left a comment

Choose a reason for hiding this comment

chengduoZH commented Sep 25, 2017

qingqing01 Sep 18, 2017 •

edited

Loading

qingqing01 Sep 18, 2017 •

edited

Loading