Add axis for `mul_op` and `rowwise_add_op` #3888

JiayiFeng · 2017-09-05T18:17:33Z

Add a global function FlattenToMatrix to convert a tensor to a matrix.
Add attributes x_num_col_dims and y_num_col_dims for mul_op and adjust its InferShape and kernel computation. num_col_dims mean how many dimensions will be producted togother to build the result matrix's first dimension.
e.g. [2,3,4,5,6] num_col_dims=3 ====> [24, 30]
Add unit tests for cases that mul_op takes tensors as inputs
Add axis for rowwise_add_op
Add unit tests for cases that rowwise_add_op takes tensors as inputs

… dev_add_axis

reyoung · 2017-09-05T18:20:15Z

paddle/framework/eigen.h

+    PADDLE_ENFORCE(num_row_dims > 0 && num_row_dims < rank,
+                   "`num_row_dims` must be between (0, rank_of_tensor).");
+    return EigenMatrix::From(
+        tensor, make_ddim({static_cast<int>(


make_ddim could be removed, just {0, 10} is OK.

Maybe add a method in class DDim, such as

class DDim { public: Dim<2> FlattenToMat(int numFlattenDims) const; };

QiJune · 2017-09-06T03:40:00Z

Whether should we add a reshape operator? if user want to multiply two tensors, he should reshape the tensor to matrix first.

JiayiFeng · 2017-09-06T03:46:15Z

@QiJune It's a feasible way, but might be slow and consume more memory.

qingqing01 · 2017-09-06T05:04:42Z

paddle/framework/attribute.h

+ public:
+  explicit EqualLargerThanChecker(T lower_bound) : lower_bound_(lower_bound) {}
+  void operator()(T& value) const {
+    PADDLE_ENFORCE(value >= lower_bound_, "equal_larger_than check fail");


check fail -> check fails

qingqing01 · 2017-09-06T05:37:01Z

paddle/framework/tensor_impl.h

@@ -148,5 +148,13 @@ inline Tensor& Tensor::Resize(const DDim& dims) {

 inline const DDim& Tensor::dims() const { return dims_; }

+template <typename T>
+inline Tensor FlattenToMatrix(const Tensor& src, int num_row_dims) {


It's better to add the explanation for num_row_dims.

num_row_dims is not easy to use, so I use num_col_dms instead. And comments have been added.

qingqing01 · 2017-09-06T05:41:24Z

paddle/operators/mul_op.cc

+    AddAttr<int>(
+        "x_num_row_dims",
+        "mul_op can take tensors with more than two dimensions as input `X`, "
+        "in that case, tensors will be flattened to a matrix. The matrix's "


flattened -> reshaped? In the Numpy, flatten means converting to a vector.

qingqing01 · 2017-09-06T05:54:40Z

paddle/operators/mul_op.cc

+        "mul_op can take tensors with more than two dimensions as input `X`, "
+        "in that case, tensors will be flattened to a matrix. The matrix's "
+        "second dimension(row length) will be the product of tensor's last "
+        "`num_row_dims` dimensions, and the matrix's first dimension(column "


second dimension(row length)
matrix's first dimension(column length)

matrix's first dimension是 dims[0]? second dimension是dims[1]吗？如果是，matrix's first dimension表示的row length(也就是height)， second dimension表示的是col length(也就是width)。

我想表达的row length的意思是“行的长度”，所以似乎应该是width？

qingqing01 · 2017-09-06T06:09:59Z

paddle/operators/mul_op.cc

+        "`num_row_dims` dimensions, and the matrix's first dimension(column "
+        "length) will be the product of tensor's first `rank - num_row_dims` "
+        "dimensions.")
+        .SetDefault(1)


依据上面的描述，和最常用的情况不符合，最常用的是reshape成：height = dims[0], width = product(dims[1:])

已经修改，把参数从num_raw_dims改成了num_col_dims，表示乘起来的前面维度的数目

qingqing01 · 2017-09-06T06:11:07Z

paddle/operators/mul_op.h

@@ -2,13 +2,13 @@

   Licensed under the Apache License, Version 2.0 (the "License");
   you may not use this file except in compliance with the License.
-   You may obtain a copy of the License at
+   you may obtain a copy of the License at


qingqing01 · 2017-09-06T06:11:17Z

paddle/operators/mul_op.h


   http://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
-   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   WITHOUT WARRANTIES OR CONDITIONS OF ANy KIND, either express or implied.


qingqing01 · 2017-09-06T06:14:47Z

paddle/operators/mul_op.h

-    z->mutable_data<T>(context.GetPlace());
+    const Tensor* x = context.Input<Tensor>("X");
+    const Tensor* y = context.Input<Tensor>("Y");
+    Tensor* Z = context.Output<Tensor>("Out");


… dev_add_axis

reyoung · 2017-09-06T18:46:50Z

paddle/framework/ddim.cc

+DDim flatten_to_2d(const DDim& src, int num_col_dims) {
+  int rank = src.size();
+  return make_ddim(
+      {static_cast<int>(product(slice_ddim(src, 0, num_col_dims))),


Well, it seems that another PR changes int -> int64_t for ddim.

I forgot to change here. Thanks!

reyoung · 2017-09-06T18:48:41Z

paddle/framework/eigen.h


 template <typename T, int MajorType = Eigen::RowMajor,
          typename IndexType = Eigen::DenseIndex>
 struct EigenVector : public EigenTensor<T, 1, MajorType, IndexType> {
  // Flatten reshapes a Tensor into an EigenVector.
  static typename EigenVector::Type Flatten(Tensor& tensor) {
-    return EigenVector::From(
-        tensor, make_ddim({static_cast<int>(product(tensor.dims_))}));
+    return EigenVector::From(tensor, {static_cast<int>(product(tensor.dims_))});


int -> int64

size_t or ssize_t is better, please do not mix use of int64 and size_t

product() retuens int64_t now, so static_cast can be removed.

reyoung · 2017-09-06T18:48:51Z

paddle/framework/eigen.h

  }

  static typename EigenVector::ConstType Flatten(const Tensor& tensor) {
-    return EigenVector::From(
-        tensor, make_ddim({static_cast<int>(product(tensor.dims_))}));
+    return EigenVector::From(tensor, {static_cast<int>(product(tensor.dims_))});


int -> int64

reyoung · 2017-09-06T18:49:02Z

paddle/framework/ddim.cc

+}
+
+DDim flatten_to_1d(const DDim& src) {
+  return make_ddim({static_cast<int>(product(src))});


int -> int64

reyoung · 2017-09-06T18:50:02Z

paddle/framework/eigen_test.cc

+TEST(Eigen, MatrixReshape) {
+  Tensor t;
+  float* p =
+      t.mutable_data<float>(make_ddim({2, 3, 6, 4}), platform::CPUPlace());


make_ddim is not needed, just t.mutable_data<float>({2, 3, 6, 4}) is cool.

reyoung · 2017-09-06T18:51:16Z

paddle/framework/tensor_impl.h

@@ -148,5 +148,13 @@ inline Tensor& Tensor::Resize(const DDim& dims) {

 inline const DDim& Tensor::dims() const { return dims_; }

+template <typename T>
+inline Tensor ReshapeToMatrix(const Tensor& src, int num_col_dims) {


inline is not needed in class method. It will be compiler's choice whether inline or not.

It's not a class method. It's a global function.

reyoung · 2017-09-06T18:51:33Z

paddle/framework/tensor_test.cc

+  using namespace paddle::framework;
+  using namespace paddle::platform;
+  Tensor src;
+  int* src_ptr = src.mutable_data<int>(make_ddim({2, 3, 4, 9}), CPUPlace());


make_ddim is not needed.

reyoung · 2017-09-06T18:52:24Z

paddle/operators/mul_op.cc

-                      ctx.op().Input("Y"));
+    auto x_dims = ctx.Input<Tensor>("X")->dims();
+    auto y_dims = ctx.Input<Tensor>("Y")->dims();
+    int x_num_col_dims = GetAttr<int>("x_num_col_dims");


Sorry... I just merge a PR, change GetAttr to Attr. Since all method in Op is Input, Output. Not GetInput or GetOutput.

reyoung · 2017-09-06T18:55:21Z

paddle/operators/mul_op.cc

@@ -47,6 +56,23 @@ class MulOpMaker : public framework::OpProtoAndCheckerMaker {
    AddInput("X", "The first input of mul op");
    AddInput("Y", "The second input of mul op");
    AddOutput("Out", "The output of mul op");
+    AddAttr<int>(


There is a very useful syntax in C++ 11.

AddAttr<int>("x_num_col_dims", R"DOC(mul_op can take ... .... )DOC");

R"LABEL(...)LABEL" is just like python's """...""". which LABEL is a custom label to identify where the string begins and ends.

See http://en.cppreference.com/w/cpp/language/string_literal

Got it, Thank you!

reyoung · 2017-09-06T18:56:01Z

paddle/operators/mul_op.cc

-                   "Out@GRAD M X N must equal to Y dims 1, N ");
+
+    auto x_mat_dims =
+        framework::flatten_to_2d(x_dims, GetAttr<int>("x_num_col_dims"));


GetAttr->Attr

reyoung · 2017-09-06T18:56:09Z

paddle/operators/mul_op.cc

+    auto x_mat_dims =
+        framework::flatten_to_2d(x_dims, GetAttr<int>("x_num_col_dims"));
+    auto y_mat_dims =
+        framework::flatten_to_2d(y_dims, GetAttr<int>("y_num_col_dims"));


GetAttr->Attr

reyoung · 2017-09-06T18:58:49Z

paddle/operators/rowwise_add_op.cc

+        x_dims.size(), b_dims.size(),
+        "The rank of input `X` must be larger than the one of input `b`.");
+
+    int num_col_dims = x_dims.size() - b_dims.size();


Interesting implementation here. So the rowwise_add's num_col_dims is decided by the rank difference.

Yes. It makes sure that b is flattened to a vector.

reyoung

Excellent Job!

But sorry for I just merging A PR change GetAttr -> Attr. So please merge develop branch before merge.

dzhwinter · 2017-09-06T20:30:37Z

paddle/framework/attribute.h

+};
+
+template <typename T>
+class EqualLargerThanChecker {


The name is better to compatible with gtest. Such as CHECK_GE or something?

EqualLargerThan is a function, not a macro, so the name shall not be too short.

dzhwinter · 2017-09-06T20:31:11Z

paddle/framework/attribute.h

+ public:
+  explicit EqualLargerThanChecker(T lower_bound) : lower_bound_(lower_bound) {}
+  void operator()(T& value) const {
+    PADDLE_ENFORCE(value >= lower_bound_, "equal_larger_than check fails.");


PADDLE_ENFORCE_GE(xxx, xxx, "comment")

dzhwinter · 2017-09-06T20:31:56Z

paddle/framework/eigen.h


 template <typename T, int MajorType = Eigen::RowMajor,
          typename IndexType = Eigen::DenseIndex>
 struct EigenVector : public EigenTensor<T, 1, MajorType, IndexType> {
  // Flatten reshapes a Tensor into an EigenVector.
  static typename EigenVector::Type Flatten(Tensor& tensor) {
-    return EigenVector::From(
-        tensor, make_ddim({static_cast<int>(product(tensor.dims_))}));
+    return EigenVector::From(tensor, {static_cast<int>(product(tensor.dims_))});


size_t or ssize_t is better, please do not mix use of int64 and size_t

dzhwinter · 2017-09-06T20:34:01Z

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

+            max_relative_error=0.5,
+            no_grad_set={"Y"})
+
+
 # TODO(dzh,qijun) : mulgrad test case need transpose feature of blas library


this line of comment can be removed.

… dev_add_axis

reyoung

Excellent

JiayiFeng added 4 commits September 4, 2017 16:55

WIP

e76fa85

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

86655cb

… dev_add_axis

Add global function FalttenToMatrix and add axis for MulOp

af0264a

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

69fbc54

… dev_add_axis

JiayiFeng requested a review from reyoung September 5, 2017 18:17

reyoung reviewed Sep 5, 2017

View reviewed changes

JiayiFeng added 2 commits September 5, 2017 15:20

Add global function flatten_to_2d()

d71396b

Add unit tests for cases that mul_op takes tensors as inputs

e168fc4

JiayiFeng changed the title ~~[WIP] Add axis for operators~~ Add axis for mul_op and rowwise_add_op Sep 5, 2017

Add axis for rowwise_add_op

256d6a3

JiayiFeng requested review from qingqing01 and QiJune September 6, 2017 03:25

qingqing01 reviewed Sep 6, 2017

View reviewed changes

JiayiFeng added 2 commits September 6, 2017 10:53

Follow comments

f2a66ff

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

823bdd6

… dev_add_axis

reyoung reviewed Sep 6, 2017

View reviewed changes

Fix bug

3d62c6d

reyoung reviewed Sep 6, 2017

View reviewed changes

reyoung previously approved these changes Sep 6, 2017

View reviewed changes

dzhwinter reviewed Sep 6, 2017

View reviewed changes

JiayiFeng added 2 commits September 6, 2017 18:40

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

0c13660

… dev_add_axis

Follow comments

5aacd64

JiayiFeng dismissed reyoung’s stale review via 5aacd64 September 7, 2017 01:44

JiayiFeng added 2 commits September 6, 2017 19:21

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

d7c8bdc

… dev_add_axis

Follow comments

b744430

JiayiFeng requested a review from wangkuiyi September 7, 2017 05:04

Move some comments to .h file

1d9a4d2

qingqing01 previously approved these changes Sep 7, 2017

View reviewed changes

JiayiFeng added 2 commits September 7, 2017 10:22

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

f6e72c9

… dev_add_axis

test

b6a4666

JiayiFeng dismissed qingqing01’s stale review via b6a4666 September 7, 2017 21:37

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

856611c

… dev_add_axis

reyoung approved these changes Sep 8, 2017

View reviewed changes

JiayiFeng merged commit 544458e into PaddlePaddle:develop Sep 8, 2017

JiayiFeng deleted the dev_add_axis branch September 8, 2017 04:22

Add axis for mul_op and rowwise_add_op #3888

Add axis for mul_op and rowwise_add_op #3888

Conversation

JiayiFeng commented Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiJune commented Sep 6, 2017

JiayiFeng commented Sep 6, 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

qingqing01 Sep 6, 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

reyoung 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

reyoung left a comment

Choose a reason for hiding this comment

Add axis for `mul_op` and `rowwise_add_op` #3888

Add axis for `mul_op` and `rowwise_add_op` #3888

JiayiFeng commented Sep 5, 2017 •

edited

Loading

JiayiFeng commented Sep 6, 2017 •

edited

Loading

qingqing01 Sep 6, 2017 •

edited

Loading