deconv op implementing ... #4739

zchen0211 · 2017-10-12T01:29:39Z

No description provided.

… develop

chengduoZH · 2017-10-20T02:01:49Z

paddle/operators/deconv2d_op.cc

+
+  for (int i = 0; i < paddings.size(); ++i) {
+    PADDLE_ENFORCE_EQ(paddings[i], 0, "No Padding allowed in deconv op.");
+  }


This check should be placed in "Deconv2DOpMaker", the current attribute checker doesn't support 'vector' type. @Canpio

For vector is not supported by AttrChecker right now, we can only do check jobs in the infer shape.

chengduoZH · 2017-10-20T02:04:50Z

paddle/operators/deconv2d_op.cc

+
+  PADDLE_ENFORCE_EQ(in_dims.size(), 4, "Deconv2DOp input should be 4-D.");
+  PADDLE_ENFORCE_EQ(filter_dims.size(), 4, "Deconv2DOp filter should be 4-D.");
+  PADDLE_ENFORCE_EQ(in_dims[1], filter_dims[0],


"Deconv2DOp filter should be 4-D." -> "Deconv2DOp filter should be 4-D tensor."

chengduoZH · 2017-10-20T02:11:02Z

paddle/operators/deconv2d_op.cc

+      "The input tensor of deconvolution operator. "
+      "The format of input tensor is NMHW. Where N is batch size, M is the "
+      "number of input channels, H and W is the height and width of image.");
+  AddInput("Filter",


Would it be better to change 'NMHW' to 'NCHW'?
Both conv and pooling use NCHW

The Deconv case is a little different from Conv case. Like in Caffe2, Conv2d use NCHW for input and MCHW for filter and produces a tensor of shape NMHW; Caffe2 Deconv applies NCHW for input, CMHW for filter and produces output tensor with shape NMHW. I will make it clear in my codes.

chengduoZH · 2017-10-20T02:17:57Z

paddle/operators/deconv2d_op.cc

+            "The format of output tensor is also NCHW.");
+  AddAttr<std::vector<int>>("strides", "strides of deconvolution operator.")
+      .SetDefault({1, 1});
+  AddAttr<std::vector<int>>("paddings", "paddings of deconvolution operator.")


Attribute checker should be placed here.

As @Canpio said, for current version to pass, we temporarily put our check here.

chengduoZH · 2017-10-20T02:28:51Z

paddle/operators/deconv2d_op.h

+ public:
+  void Compute(const framework::ExecutionContext& context) const override {
+    const Tensor* input = context.Input<Tensor>("Input");
+    // filter will be reshaped, so we do not use constant pointer here


Instead of "The filter will be reshaped in the calculations, so it should not be constant pointer." ?

chengduoZH · 2017-10-20T02:40:03Z

paddle/operators/deconv2d_op.h

+        context.Input<Tensor>(framework::GradVarName("Output"));
+
+    // For filter, we do not use const pointer b/c we will do reshape
+    // but we should avoid modifying its value


Add period.

chengduoZH · 2017-10-20T02:58:46Z

paddle/operators/deconv2d_op.h

+        context.Output<Tensor>(framework::GradVarName("Filter"));
+
+    std::vector<int> strides = context.Attr<std::vector<int>>("strides");
+    // Actually, no paddings and groups allowed in deconv


Add period.

chengduoZH · 2017-10-20T03:02:07Z

paddle/operators/deconv2d_op.h

+
+    int C = output_grad->dims()[1];  // output channels
+    int O_H = output_grad->dims()[2];
+    int O_W = output_grad->dims()[3];


variable names style

chengduoZH · 2017-10-20T03:04:50Z

paddle/operators/deconv2d_op.cc

+      "number of input channels, H and W is the height and width of image.");
+  AddInput("Filter",
+           "The filter tensor of deconvolution operator."
+           "The format of the filter tensor is MCHW, where M is the number of "


"MCHW" - >"NCHW"

chengduoZH · 2017-10-20T03:06:38Z

paddle/operators/deconv2d_op.cc

+           "input image channels, C is the number of output image channels, "
+           "H and W is height and width of filter. "
+           "We enforce groups number == 1 and padding == 0 in our "
+           "deconvolution Scenario.");


We enforce groups number == 1 and padding == 0 in ~~our~~ deconvolution Scenario.

qingqing01 · 2017-10-20T08:22:05Z

paddle/operators/deconv2d_op.cc

+namespace paddle {
+namespace operators {
+
+void Deconv2DOp::InferShape(framework::InferShapeContext* ctx) const {


From the comments: tensorflow/tensorflow#256 (comment)

How about rename Conv2DTranspose?

Great suggestion!

qingqing01 · 2017-10-20T08:25:09Z

paddle/operators/deconv2d_op.cc

+      "Input",
+      "The input tensor of deconvolution operator. "
+      "The format of input tensor is NMHW. Where N is batch size, M is the "
+      "number of input channels, H and W is the height and width of image.");


"(Tensor) The input tensor of transposed 2D convolution operator. "

The () is used to denote the type, same as the following annotations.

NMHW -> NCHW

qingqing01 · 2017-10-20T08:26:46Z

paddle/operators/deconv2d_op.h

+
+#pragma once
+
+#include "glog/logging.h"


remove glog.

qingqing01 · 2017-10-20T08:37:30Z

paddle/operators/deconv2d_op.h

+
+    for (int i = 0; i < N; i++) {
+      // batch with size (M, H * W)
+      Tensor input_batch = input->Slice<T>(i, i + 1).Resize(input_matrix_shape);


Update code, since the Slice removed the template T.

qingqing01 · 2017-10-20T08:39:08Z

paddle/operators/deconv2d_op.h

+
+    std::vector<int> strides = context.Attr<std::vector<int>>("strides");
+
+    // no paddings and groups allowed in deconv


If to do in next PR, add TODO comments.

mkliegl

LGTM. As we discussed separately, it may be worth trying to speed up GPU performance by using CUDNN convolution kernels in a future PR.

mkliegl · 2017-10-20T18:01:30Z

paddle/operators/deconv2d_op.h

+    // but will be reshaped into a two-dimensional matrix shape
+    // to call the matrix multiplication interface.
+    Tensor col_matrix = col;
+    col_matrix.Resize(col_matrix_shape);


That copy assign works as intended, but it looks a little unnatural to me at first glance, since for e.g. std::vector, copy assign copies the data. However, copy assignment does share data in this case because the data is stored inside a std::shared_ptr inside the Tensor class. Nevertheless, I would suggest the more explicit:

Tensor col_matrix; col_matrix.ShareDataWith(col);

(I realize this is carried over from conv2d_op.h - maybe you could change it there, too?)

Great thanks!

mkliegl · 2017-10-20T18:04:46Z

paddle/operators/deconv2d_op.h

+    // input need to compute gradient
+    if (input_grad) {
+      Tensor col_matrix = col;
+      DDim col_matrix_shape = {C * K_H * K_W, H * W};


See above comment. I would prefer the more explicit:

Tensor col_matrix; col_matrix.ShareDataWith(col);

… develop

chengduoZH · 2017-10-24T01:39:24Z

paddle/operators/conv2dtranspose_op.cc

+  PADDLE_ENFORCE_EQ(filter_dims.size(), 4,
+                    "Conv2DTransposeOp filter should be 4-D tensor.");
+  PADDLE_ENFORCE_EQ(in_dims[1], filter_dims[0],
+                    "input and kernel input dimension should be equal.");


->"In Conv2DTransposeOp, The input channel should be the same as the number of filters."

chengduoZH · 2017-10-24T02:17:02Z

paddle/operators/conv2dtranspose_op.cc

+The convolution transpose operation calculates the output based on the input, filter
+and strides, paddings, groups parameters. The size of each dimension of the
+parameters is checked in the infer-shape.
+)DOC");


Better to give how to calculate the output height/width according to the input height/with, padding and stride size.

The doc of Pytorch is good.

http://pytorch.org/docs/master/nn.html#convtranspose2d

chengduoZH · 2017-10-24T02:24:56Z

paddle/operators/conv2dtranspose_op.h

+    std::vector<int> strides = context.Attr<std::vector<int>>("strides");
+
+    // TODO(Zhuoyuan): Paddings can be added in future.
+    // groups will alway be disabled in conv2dtranspose.


The attribution of group should be available in conv2dtranspose. Reference Pytorch

chengduoZH · 2017-10-24T02:28:49Z

paddle/operators/conv2dtranspose_op.h

+    for (int i = 0; i < batch_size; i++) {
+      // batch with size (M, h * w)
+      Tensor input_batch = input->Slice(i, i + 1).Resize(input_matrix_shape);
+      // filter size: (M, c * k_h * k_w)


You can delete this comment or write in line 119.

chengduoZH · 2017-10-24T02:30:00Z

paddle/operators/conv2dtranspose_op.h

+        // batch with size (c, o_h * o_w)
+        Tensor output_grad_batch =
+            output_grad->Slice(i, i + 1).Resize(output_shape);
+        // filter of size (m, c * k_h * k_w)


Same as above.

chengduoZH · 2017-10-24T02:36:16Z

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

+        self.dilations = [1, 1]
+        self.input_size = [2, 3, 5, 5]  # NCHW
+        f_c = self.input_size[1]
+        self.filter_size = [f_c, 6, 3, 3]


If the calculation is not very time-consuming, you can write several border test examples.

zchen0211 added 3 commits October 11, 2017 17:34

deconv op

532f38d

deconv

1dd6dbb

final deconv

c4d232c

zchen0211 added the OpPorting label Oct 12, 2017

zchen0211 added 2 commits October 12, 2017 13:42

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

416f590

… develop

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

da399ae

… develop

qingqing01 mentioned this pull request Oct 13, 2017

Review operators required by books. #4786

Closed

36 tasks

zchen0211 added 11 commits October 13, 2017 14:05

deconv

652f182

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

451863d

… develop

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

98dccc9

… develop

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

80ebc8d

… develop

deconv impl

5ec55e7

deconv

43aad98

deconv2d impl in full

e8cd4b7

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

e59ca75

… develop

deconv

d97a732

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

c33575a

… develop

deconv

7eeaae1

zchen0211 requested review from mkliegl, qingqing01, chengduoZH and reyoung October 20, 2017 00:05

chengduoZH requested changes Oct 20, 2017

View reviewed changes

deconv2d

8e55736

qingqing01 reviewed Oct 20, 2017

View reviewed changes

jacquesqiao self-requested a review October 20, 2017 18:11

mkliegl reviewed Oct 20, 2017

View reviewed changes

zchen0211 added 3 commits October 20, 2017 11:48

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

502e725

… develop

deconv

64c5ecb

deconv -> conv transpose

b3ab3ce

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

cc5e118

… develop

chengduoZH approved these changes Oct 24, 2017

View reviewed changes

zchen0211 merged commit 8fdc315 into PaddlePaddle:develop Oct 24, 2017

chengduoZH reviewed Oct 24, 2017

View reviewed changes


		std::vector<int> strides = context.Attr<std::vector<int>>("strides");

		// no paddings and groups allowed in deconv


		#pragma once

		#include "glog/logging.h"

deconv op implementing ... #4739

deconv op implementing ... #4739

Conversation

zchen0211 commented Oct 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

chengduoZH Oct 20, 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

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

mkliegl left a comment

Choose a reason for hiding this comment

mkliegl Oct 20, 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

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

chengduoZH Oct 20, 2017 •

edited

Loading

mkliegl Oct 20, 2017 •

edited

Loading