Add sequence_conv_op and sequence_projection functor #4814

chengduoZH · 2017-10-15T10:52:06Z

chengduoZH · 2017-10-23T09:12:38Z

Because seq_project is only used in seq_conv, seq_project should be written in functor form.

dzhwinter · 2017-10-26T04:06:48Z

I think we can merge it first and review the code the same time. @chengduoZH Please continue to polish the code based on the comments.

And, please split PR into small ones. Such a big PR will take a long time to review.
Thanks.

chengduoZH · 2017-10-26T04:48:51Z

@dzhwinter Ok!

qingqing01 · 2017-10-25T06:10:56Z

paddle/operators/math/sequence_project.h

+ * \param col           Col data.
+ * \param inShape       The shape of Col data,
+ *                      [minibatch, 1].
+ * \param inShape       A float LoDTensor.


Why are so many inShape?

qingqing01 · 2017-10-25T06:12:54Z

paddle/operators/math/sequence_project.h

+ * \param inShape       A float LoDTensor.
+ *
+ * For a mini-batch of 2 variable lengths sentences, containing 3, and 1
+ * time-steps:


Line 34 says this function is used for one sequence, but the example here has variable lengths sentences. Please to keep consistent.

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

paddle/operators/math/sequence_project.h

+               sequence_width});  // output_height, output_width,
+          // input_channels, filter_height, filter_width
+
+          out_t.Resize(framework::make_ddim(output_shape));


Can remove the framework::make_ddim, since the std::vector can be automatically converted to DDim, the same below

qingqing01 · 2017-10-26T08:26:29Z

paddle/operators/sequence_conv_op.cc

+    PADDLE_ENFORCE(
+        filter_dims[0] == context_length && filter_dims[1] == in_dims[1],
+        "Filter's shape should be (context_length x "
+        "number_of_input_features).");


The filter shape is not right.

假如：context_length = 3, 输入hidden size = D，输出的hidden size = H
Filter: [3D, H]

qingqing01 · 2017-10-26T08:31:49Z

paddle/operators/sequence_conv_op.cc

+    }
+
+    in_dims[1] = 1;
+    ctx->SetOutputDim("Out", in_dims);


The output shape is not right.

依据上面假设输出dims[1] = H。

Also should set LoD for output.

qingqing01 · 2017-10-26T08:41:51Z

paddle/operators/sequence_conv_op.h

+    // Because if padding_trainable is false, padding data should be zeros.
+    auto temp = framework::EigenVector<T>::Flatten(col);
+    temp.device(context.GetEigenDevice<Place>()) =
+        temp.constant(static_cast<T>(0));


调用math:: SetConstant置零： https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/math/math_function.h#L97

qingqing01 · 2017-10-26T08:42:24Z

paddle/operators/sequence_conv_op.h

+
+    filter.Resize(framework::make_ddim({context_length * sequence_width, 1}));
+    math::matmul<Place, T>(context.device_context(), col, false, filter, false,
+                           T(1.0), out, T(0.0));


T(1.0) -> static_cast<T>(1.0)

qingqing01 · 2017-10-26T08:43:19Z

paddle/operators/sequence_conv_op.h

+      // Because if padding_trainable is false, padding data should be zeros.
+      auto temp = framework::EigenVector<T>::Flatten(col);
+      temp.device(context.GetEigenDevice<Place>()) =
+          temp.constant(static_cast<T>(0));


调用math:: SetConstant置零： https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/math/math_function.h#L97

qingqing01 · 2017-10-26T08:44:48Z

paddle/operators/sequence_conv_op.h

+      functor(context.device_context(), filter_g, 0);
+
+      Tensor filter_grad_ = *filter_g;
+      LoDTensor out_grad_ = *out_g;


out_grad_ -> out_grad

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

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

+        output_dim = self.outputs['Out'].shape
+        filter.shape = filter_dim[0] * filter_dim[1]
+        self.outputs['Out'].shape = (output_dim[0], )
+        np.dot(out, filter, out=self.outputs['Out'])


Python单测forward实现，觉得避免和C++ Code一致，避免采用先展开后矩阵乘的形式，可以是Conv原本实现形式。

Python单测是根据之前paddle改写过来的，context_project_functor是先经过im2col再通过矩阵乘得到的，这两种方式并不太一样

qingqing01

Since the Python API needs this op, approve it. But still need to modify later.

qingqing01 · 2017-10-26T12:38:40Z

paddle/operators/math/context_project.h

+                  framework::Tensor& col, bool padding_trainable,
+                  int context_start, int context_length, int context_stride,
+                  int up_pad, int down_pad, bool gradient, bool input_grad,
+                  bool pad_grad) {


觉得将projection和un-projection的过程混合在一起，代码逻辑不够清晰。

分开写也是可以的，不过显得代码有点冗余，我再想想办法

qingqing01 · 2017-10-26T12:38:46Z

paddle/operators/math/context_project.h

+ * \param in            Input data.
+ * \param Shape         The shape of Input data,
+ *                      [minibatch, number_of_input_features].
+ * \param type          A float LoDTensor.


Remove the type, there is no meaning here.

The argument type in the following function is clear.

Done. #5130

qingqing01 · 2017-10-26T12:38:48Z

paddle/operators/math/context_project.h

+
+ * \param in            Input data.
+ * \param Shape         The shape of Input data,
+ *                      [minibatch, number_of_input_features].


number_of_input_features -> input_hidden_size

Done. #5130

qingqing01 · 2017-10-26T12:40:36Z

paddle/operators/sequence_conv_op.cc

+        "this LoDTensor is a matrix with shape (T, D), where, T is the "
+        "total time steps in this mini-batch, D is the output feature size.");
+
+    AddAttr<bool>("padding_trainable",


paddingTrainable, please to see our name convention.

Done. #5130

qingqing01 · 2017-10-26T12:40:51Z

paddle/operators/sequence_conv_op.cc

+                  "(bool, default false) the padding data of SequenceConvOp "
+                  "is trainable or not.")
+        .SetDefault(false);
+    AddAttr<int>("context_length",


contextLength

Done. #5130

qingqing01 · 2017-10-26T12:41:01Z

paddle/operators/sequence_conv_op.cc

+                 "height of the convolution kernel.")
+        .SetDefault(3)
+        .GreaterThan(0);
+    AddAttr<int>("context_start",


contextStart

Done. #5130

qingqing01 · 2017-10-26T12:41:12Z

paddle/operators/sequence_conv_op.cc

+                 "represents the beginning of the convolution of the number of "
+                 "rows of sequence, which can be negative.")
+        .SetDefault(0);
+    AddAttr<int>("context_stride",


contextStride

Done. #5130

qingqing01 · 2017-10-26T12:42:50Z

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

+        del idx[0]
+        self.lod = [[0] + np.sort(random.sample(idx, 8)).tolist() +
+                    [self.input_size[0]]]
+        self.output_represention = 8  # output feature size


Need unit testing for the case self.context_stride > 1

Currently, seq_conv_op only supports self.context_stride = 1.

chengduoZH added the OpPorting label Oct 15, 2017

qingqing01 mentioned this pull request Oct 15, 2017

Review operators required by books. #4786

Closed

36 tasks

chengduoZH force-pushed the Add_sequence_project_op branch 3 times, most recently from 1faad45 to 4de6294 Compare October 18, 2017 05:23

Add sequence_project_op (use im2col)

1e60c9b

chengduoZH force-pushed the Add_sequence_project_op branch from 4de6294 to 1e60c9b Compare October 18, 2017 05:25

chengduoZH requested review from lcy-seso, qingqing01 and hedaoyuan October 18, 2017 05:27

refine im2col (up_pad,down_pad)

40688d2

chengduoZH force-pushed the Add_sequence_project_op branch from 40688d2 to 4b0ec8f Compare October 18, 2017 07:06

chengduoZH mentioned this pull request Oct 19, 2017

operators needed by recommender system #4780

Closed

6 tasks

fix sequence_project_op forward and backward

834b82f

chengduoZH force-pushed the Add_sequence_project_op branch from 4b0ec8f to 834b82f Compare October 21, 2017 05:02

remove conflict

4d112b7

chengduoZH force-pushed the Add_sequence_project_op branch from 8d6f296 to 6d375e5 Compare October 21, 2017 09:04

clean gradient data

6246be2

chengduoZH force-pushed the Add_sequence_project_op branch 4 times, most recently from bf2feb2 to b0092ea Compare October 22, 2017 03:14

fix backward

4c19f9f

chengduoZH force-pushed the Add_sequence_project_op branch 5 times, most recently from dd4a738 to 5cd8a9a Compare October 23, 2017 03:36

fix doc format and unit test

ce96057

chengduoZH force-pushed the Add_sequence_project_op branch from 5cd8a9a to ce96057 Compare October 23, 2017 03:40

chengduoZH added 2 commits October 23, 2017 20:15

Add sequence_project_functor

0ab2c43

Add sequence_conv_op

f2ccef2

chengduoZH force-pushed the Add_sequence_project_op branch 4 times, most recently from f2da6c2 to c2eb73e Compare October 24, 2017 03:04

Add unit test

154dbb4

chengduoZH force-pushed the Add_sequence_project_op branch 2 times, most recently from 8d63828 to 6ce31f6 Compare October 24, 2017 07:30

fix doc and remove useless code

4c6bccb

chengduoZH force-pushed the Add_sequence_project_op branch from 932e0f7 to 4c6bccb Compare October 24, 2017 08:34

chengduoZH added 2 commits October 24, 2017 18:00

remove conflict

b15c69f

fix unit test

6f02fe7

chengduoZH changed the title ~~Add sequence_project_op~~ Add sequence_conv_op and sequence_projection functor Oct 24, 2017

fix functor

05239b6

dzhwinter mentioned this pull request Oct 26, 2017

"add sequence conv layer" #5117

Merged

refine code

dcb3da5

chengduoZH force-pushed the Add_sequence_project_op branch from bcdaae5 to dcb3da5 Compare October 26, 2017 08:06

qingqing01 reviewed Oct 26, 2017

View reviewed changes

chengduoZH force-pushed the Add_sequence_project_op branch from 5e60d24 to 4ff4f0f Compare October 26, 2017 11:15

follow comments

99c6f44

chengduoZH force-pushed the Add_sequence_project_op branch from 4ff4f0f to 99c6f44 Compare October 26, 2017 11:52

qingqing01 approved these changes Oct 26, 2017

View reviewed changes

chengduoZH merged commit 8e3ecf5 into PaddlePaddle:develop Oct 26, 2017

chengduoZH mentioned this pull request Oct 26, 2017

fix_sequence_conv_op #5130

Merged

Add sequence_conv_op and sequence_projection functor #4814

Add sequence_conv_op and sequence_projection functor #4814

Conversation

chengduoZH commented Oct 15, 2017 • edited Loading

chengduoZH commented Oct 23, 2017

dzhwinter commented Oct 26, 2017 • edited Loading

chengduoZH commented Oct 26, 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

qingqing01 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

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 commented Oct 15, 2017 •

edited

Loading

dzhwinter commented Oct 26, 2017 •

edited

Loading