Add pad op #3765

wanghaoshuang · 2017-08-30T12:01:48Z

… pad_op Conflicts: paddle/pybind/pybind.cc

QiJune · 2017-08-30T12:35:04Z

paddle/operators/pad_op.h

+    // Eigen::DenseIndex>> X_tensor = EigenTensor<T, 2>::From(*X);
+    // Eigen::TensorMap<Eigen::Tensor<T, 2, Eigen::RowMajor, Eigen::DenseIndex>>
+    // Out_tensor = EigenTensor<T, 2>::From(*Out);
+    EigenTensor<T, dims.size()>::ConstType X_tensor =


这里的dims的值需要在编译期就确定了，不可以在这里通过传递一个变量来运行时确定。所以PadKernel需要额外多一个模板参数。

template<typename Place, typename T, int Dims> class PadKernel : public framework::OpKernel { ... EigenTensor<T, Dims>::ConstType x_tensor = EigenTensor<T, Dims>::From(*X); ... };

可以参考tensorflow的pad_op的写法

觉得应该是想TensorFlow一样，pad functor带模板Dims(或叫Rank)即可： https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/kernels/pad_op.cc#L143

不用 PadKernel不用带模板参数Dims

qingqing01 · 2017-09-01T06:09:29Z

paddle/operators/pad_op.h

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


remove this line.

qingqing01 · 2017-09-01T06:21:59Z

paddle/operators/pad_op.cc

+
+class MulOpMaker : public framework::OpProtoAndCheckerMaker {
+ public:
+  MulOpMaker(framework::OpProto *proto, framework::OpAttrChecker *op_checker)


MulOpMaker -> PadOpMaker

qingqing01 · 2017-09-01T06:23:14Z

paddle/operators/pad_op.cc

+  void InferShape(const framework::InferShapeContext &ctx) const override {
+    auto dim0 = ctx.Input<Tensor>("X")->dims();
+    auto dim1 = ctx.Output<Tensor>("Out")->dims();
+    auto paddings = GetAttr<std::vector<std::pair<int32, int32>>>("paddings");


check the paddings.size() == dim0.size()

qingqing01 · 2017-09-01T06:23:54Z

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

+        op = Operator("pad", paddings=((0, 1), (2, 3)), pad_value=0)
+        inputs = {'X': np.random.random((16, 16)).astype("float32"), }
+
+        self.check_grad(op, inputs, set(["X"]), "Out", max_relative_error=0.5)


add compare_grad to compare the CPU and GPU results.

qingqing01 · 2017-09-01T06:26:05Z

paddle/operators/pad_op.h

+    // Eigen::DenseIndex>> X_tensor = EigenTensor<T, 2>::From(*X);
+    // Eigen::TensorMap<Eigen::Tensor<T, 2, Eigen::RowMajor, Eigen::DenseIndex>>
+    // Out_tensor = EigenTensor<T, 2>::From(*Out);
+    EigenTensor<T, dims.size()>::ConstType X_tensor =


觉得应该是想TensorFlow一样，pad functor带模板Dims(或叫Rank)即可： https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/kernels/pad_op.cc#L143

不用 PadKernel不用带模板参数Dims

… pad_op Conflicts: paddle/operators/CMakeLists.txt paddle/pybind/CMakeLists.txt

qingqing01 · 2017-09-06T13:01:31Z

paddle/operators/pad_op.h

+template <typename Place, typename T, size_t D>
+void PadFunction(const framework::ExecutionContext& context) {
+  auto pads =
+      context.op().GetAttr<std::vector<std::pair<int, int>>>("paddings");


context.op(). -> context.

qingqing01 · 2017-09-06T13:03:04Z

paddle/operators/pad_op.h

+  }
+  T pad_value = context.op().GetAttr<T>("pad_value");
+
+  auto* X = context.Input<Tensor>("X");


The names of variables (including function parameters) and data members are all lowercase

https://google.github.io/styleguide/cppguide.html#Variable_Names

qingqing01 · 2017-09-06T13:03:46Z

paddle/operators/pad_op.h

+  for (int i = 0; i < pads.size(); ++i) {
+    paddings[i] = pads[i];
+  }
+  T pad_value = context.op().GetAttr<T>("pad_value");


context.op() -> context

qingqing01 · 2017-09-06T13:10:45Z

paddle/operators/pad_op.h

+        PadFunction<Place, T, 6>(context);
+        break;
+      default:
+        LOG(ERROR) << "Only ranks up to 6 supported.";


LOG(ERROR) -> PADDLE_THROW(“Only support tensor with ranks up to 6.”)

qingqing01 · 2017-09-06T13:15:24Z

paddle/operators/pad_op.h

+      default:
+        LOG(ERROR) << "Only ranks up to 6 supported.";
+    }
+  }


same problem with PadFunction

qingqing01 · 2017-09-06T13:17:13Z

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

+
+    def test_normal(self):
+        self.check_grad(
+            self.op, self.inputs, set(["X"]), "Out", max_relative_error=0.5)


remove max_relative_error=0.5. For pad op, the max_relative_error should be very small.

qingqing01 · 2017-09-06T13:20:22Z

paddle/operators/pad_op.cc

+    AddAttr<std::vector<std::pair<int, int>>>(
+        "paddings", "The padding rules for each dimension");
+    AddAttr<float>("pad_value", "The value to be padded into tensor")
+        .SetDefault(0.0f);


need more detail and meaningful comments.

#3885

qingqing01 · 2017-09-06T13:21:45Z

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

+            'Out': np.pad(self.inputs['X'],
+                          self.attrs['paddings'],
+                          mode='constant',
+                          constant_values=0)


add unit testing for other shape and padding value.

QiJune · 2017-09-08T06:31:08Z

paddle/operators/pad_op.cc

+    AddComment(R"DOC(
+Pad Operator.
+)DOC");
+    AddAttr<std::vector<std::pair<int, int>>>(


这个attribute是怎么设置成功的呀，我看attribute的定义，目前只能支持

typedef boost::variant<boost::blank, int, float, std::string, std::vector<int>, std::vector<float>, std::vector<std::string>> Attribute;

实际上是没有 std::vector<std::pair<int,int>>的类型的

We are discussing about this issue:
#3877

1. Rename variables by Google style. 2. Add more test cases. 3. Add more detail and meaningful comments. 4. Change type of "padding" to vector<int>

… pad_op

JiayiFeng · 2017-09-08T18:25:19Z

paddle/operators/pad_op.cc

+  void InferShape(const framework::InferShapeContext &ctx) const override {
+    auto dim0 = ctx.Input<Tensor>("X")->dims();
+    auto paddings = GetAttr<std::vector<int>>("paddings");
+    PADDLE_ENFORCE_EQ(dim0.size(), (int)(paddings.size() / 2),


(int)(...) is C style, please use int(...) instead.

Assume that dim_size = 2 and padding_size = 5, it is illegal because padding_size shall always be even. However, current assert can't handle them properly.

We can change it to:

PADDLE_ENFORCE_EQ(dim0.size() * 2, int(paddings.size()), ...)

JiayiFeng · 2017-09-08T18:26:26Z

paddle/operators/pad_op.cc

+ protected:
+  void InferShape(const framework::InferShapeContext &ctx) const override {
+    auto dim0 = ctx.Input<Tensor>("X")->dims();
+    auto paddings = GetAttr<std::vector<int>>("paddings");


GetAttr has been renamed to Attr.

Fixed. Thx.

JiayiFeng · 2017-09-08T18:30:07Z

paddle/operators/pad_op.cc

+
+ protected:
+  void InferShape(const framework::InferShapeContext &ctx) const override {
+    auto dim0 = ctx.Input<Tensor>("X")->dims();


I think x_dims is a more meaningful name.

JiayiFeng · 2017-09-08T18:33:07Z

paddle/operators/pad_op.cc

+                      "of input tensor.");
+    std::vector<int> dim1(dim0.size());
+    for (int i = 0; i < dim0.size(); ++i) {
+      dim1[i] = dim0[i] + paddings[i * 2] + paddings[i * 2 + 1];


out_dims is better?

JiayiFeng · 2017-09-08T18:45:34Z

paddle/operators/pad_op.cc

+    for (int i = 0; i < dim0.size(); ++i) {
+      dim1[i] = dim0[i] + paddings[i * 2] + paddings[i * 2 + 1];
+    }
+    ctx.Output<Tensor>("Out")->Resize(paddle::framework::make_ddim(dim1));


framework::make_ddim(dim1) is enough.

JiayiFeng · 2017-09-08T18:48:16Z

paddle/operators/pad_op.cc

+  PadOpMaker(framework::OpProto *proto, framework::OpAttrChecker *op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X", "The input of pad op.");
+    AddOutput("Out", "The output of pad op.");


Please add dim information into Input/output comments.
like https://github.com/PaddlePaddle/Paddle/pull/3887/files

JiayiFeng · 2017-09-08T19:02:22Z

paddle/operators/pad_op.cc

+        " 2 * dimension size of input tensor.");
+    AddAttr<float>("pad_value",
+                   "(float) default to 0; "
+                   "The value to be padded into tensor. ")


"The value to fill padded areas. "
Is this better?

… pad_op

JiayiFeng · 2017-09-11T05:56:00Z

paddle/operators/pad_op.cc

+    PADDLE_ENFORCE_NOT_NULL(ctx.InputVar(framework::GradVarName("Out")),
+                            "Input(Out@GRAD) should not be null");
+    auto x_dims = ctx.Input<Tensor>("X")->dims();
+    auto *x_grad = ctx.Output<Tensor>(framework::GradVarName("X"));


Seems that we shall also assert x_grad is not nullptr?

Sorry I made a mistake... x_grad can be nullptr. If so, we don't need to compute x_grad.

JiayiFeng · 2017-09-11T06:05:13Z

paddle/operators/pad_op.h

+  auto* x = context.Input<Tensor>("X");
+  auto* out = context.Output<Tensor>("Out");
+  out->mutable_data<T>(context.GetPlace());
+  auto dims = x->dims();


This dims seems unused.

JiayiFeng · 2017-09-11T06:07:02Z

paddle/operators/pad_op.h

+class PadKernel : public framework::OpKernel {
+ public:
+  void Compute(const framework::ExecutionContext& context) const override {
+    int dim = context.Input<Tensor>("X")->dims().size();


The number of a tensor's dim is called rank in TensorFlow. We can also use this name.

JiayiFeng · 2017-09-11T06:10:56Z

paddle/operators/pad_op.h

+        PadFunction<Place, T, 6>(context);
+        break;
+      default:
+        PADDLE_THROW("Only ranks up to 6 supported.");


"PadOp only support tensors with no more than 6 dimensions."
Just for your reference.

JiayiFeng · 2017-09-11T06:17:54Z

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

+class TestCase3(TestPadOp):
+    def initTestCase(self):
+        self.shape = (8)
+        self.paddings = [(0, 1)]


Can paddings be negative? If so, we shall add unit tests for it.

Thx. But i think negative paddings is not suggested to be used in pad op. crop op is better for your scene.

… pad_op

JiayiFeng

LGTM

JiayiFeng · 2017-09-11T18:17:17Z

paddle/operators/pad_op.cc

+    AddInput("X",
+             "The input of pad op. "
+             "The input should be a k-D tensor(k > 0 and k < 7)");
+    AddOutput("Out",


Seems that output Out will not be used in backward infershape and computing? If so, use NotInGradient() to mark it, then our graph compiler is able to optimize the memory.
See https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/minus_op.cc#L45

… pad_op

wanghaoshuang added 2 commits August 30, 2017 19:54

Add pad op

26cec83

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

2db7ded

… pad_op Conflicts: paddle/pybind/pybind.cc

wanghaoshuang requested a review from QiJune August 30, 2017 12:02

QiJune reviewed Aug 30, 2017

View reviewed changes

qingqing01 requested review from luotao1, JiayiFeng and qingqing01 August 31, 2017 02:02

qingqing01 reviewed Sep 1, 2017

View reviewed changes

qingqing01 added the OpPorting label Sep 5, 2017

wanghaoshuang added 4 commits September 6, 2017 10:58

Fix eigen error.

3eadb42

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

6684b55

… pad_op Conflicts: paddle/operators/CMakeLists.txt paddle/pybind/CMakeLists.txt

Fix some issues.

9f8e498

Fix padding attribute error.

7c30251

qingqing01 reviewed Sep 6, 2017

View reviewed changes

QiJune reviewed Sep 8, 2017

View reviewed changes

wanghaoshuang added 3 commits September 8, 2017 16:39

Refine pad op

0910a9b

1. Rename variables by Google style. 2. Add more test cases. 3. Add more detail and meaningful comments. 4. Change type of "padding" to vector<int>

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

d79e3e4

… pad_op

Fix comment

d960cbd

JiayiFeng reviewed Sep 8, 2017

View reviewed changes

wanghaoshuang added 2 commits September 11, 2017 11:46

Fix variable names and comments

c7b3478

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

e1da6c0

… pad_op

JiayiFeng reviewed Sep 11, 2017

View reviewed changes

wanghaoshuang added 3 commits September 11, 2017 14:50

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

ba8a5c1

… pad_op

Fix issues

f31217f

Fix warning log

9c929a4

JiayiFeng previously approved these changes Sep 11, 2017

View reviewed changes

JiayiFeng reviewed Sep 11, 2017

View reviewed changes

wanghaoshuang added 2 commits September 12, 2017 10:04

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

a03c684

… pad_op

fix paddle enforce check

355e35f

wanghaoshuang dismissed JiayiFeng’s stale review via 355e35f September 12, 2017 02:13

wanghaoshuang added 4 commits September 12, 2017 10:43

fix NoInGrad bug

012453e

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

d895980

… pad_op

Fix nullptr check

236a84c

Update pad op unitest

23381dd

qingqing01 approved these changes Sep 13, 2017

View reviewed changes

wanghaoshuang merged commit 08f9b72 into PaddlePaddle:develop Sep 13, 2017


		#pragma once

		#include "paddle/operators/math/math_function.h"

Add pad op #3765

Add pad op #3765

Conversation

wanghaoshuang commented Aug 30, 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

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

wanghaoshuang Sep 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiayiFeng Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

wanghaoshuang Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

JiayiFeng left a comment

Choose a reason for hiding this comment

JiayiFeng Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanghaoshuang commented Aug 30, 2017 •

edited

Loading

wanghaoshuang Sep 11, 2017 •

edited

Loading

JiayiFeng Sep 11, 2017 •

edited

Loading

wanghaoshuang Sep 11, 2017 •

edited

Loading

JiayiFeng Sep 11, 2017 •

edited

Loading