Seq expand op #4740

wanghaoshuang · 2017-10-12T02:00:42Z

fix #5000

1. Add unitest 2. Add SeqExpandOpKernel

… seq_expand_op

1. Add more comments and exmples 2. Rename repeat_lod to expand_lod 3. Remove unused head file

… seq_expand_op

QiJune · 2017-10-20T02:16:24Z

paddle/operators/seq_expand_op.cc

+      out_dim[0] = out_dim[0] * repeat;
+    }
+    PADDLE_ENFORCE(ctx->HasOutput("Out"),
+                   "Output(Out) of PadOp should not be null.");


PadOp --> SeqExpandOp

QiJune · 2017-10-20T02:27:24Z

paddle/operators/seq_expand_op.h

+    for (size_t i = 0; i < scales.size(); ++i) {
+      count = element_len * (x_lod[0][i + 1] - x_lod[0][i]);
+      for (size_t j = 0; j < scales[i]; ++j) {
+        memory::Copy(place, out_data, place, x_data, sizeof(T) * count);


In GPU, we should set a CUDA stream for copy.

#ifdef PADDLE_WITH_CUDA auto stream = reinterpret_cast<const platform::CUDADeviceContext&>( context.device_context()) .stream(); memory::Copy(...... stream); #else memory::Copy(......); #endif

QiJune · 2017-10-20T02:30:37Z

paddle/operators/seq_expand_op.h

+      Eigen::TensorMap<Eigen::Tensor<T, 1>> d_x_t(
+          d_x_data, static_cast<int>((ele_count * element_len) / repeat));
+      auto place = context.GetEigenDevice<Place>();
+      d_x_t.device(place) = d_out_t.sum(Eigen::array<int, 1>({0}));


Better change to

Eigen::array<int, 1>({{0}})

for clang compile.
https://stackoverflow.com/questions/31555584/why-is-clang-warning-suggest-braces-around-initialization-of-subobject-wmis

qingqing01

All the code is not read yet. Just part comments.

qingqing01 · 2017-10-20T09:11:06Z

paddle/operators/seq_expand_op.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput(
+        "X",
+        "The input('X') of seq_expand op. It can be LoDTensor or base Tensor.");


"(Tensor or LoDTensor) The input('X') of this operator can be a LoDTensor or a base Tensor."

qingqing01 · 2017-10-20T09:13:19Z

paddle/operators/seq_expand_op.cc

+        "It must be a LoDTensor with k-level(k>0)."
+        "This reference input is essential if 'repeat' attribute is not "
+        "configured."
+        "Input(X) will be expanded by LoD of input(Y) while repeat ==  0.");


by - > according to the

qingqing01 · 2017-10-20T09:13:32Z

paddle/operators/seq_expand_op.cc

+        "The input('X') of seq_expand op. It can be LoDTensor or base Tensor.");
+    AddInput(
+        "Y",
+        "The reference input('Y') of seq_expand op."


" (LoDTensor) The ... "

qingqing01 · 2017-10-20T10:16:57Z

paddle/framework/lod_tensor.cc

@@ -103,5 +103,34 @@ void LoDTensor::ShrinkInLevel(size_t level, size_t elem_begin,
  lod_ = new_lod;
 }

+Vector<size_t> expand_lod(Vector<size_t> level, Vector<size_t> starts,
+                          Vector<size_t> scales, bool repeat) {


Functions names: "CamelCase"

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

Now, only the seq_expand needs this function, it may be removed to seq_expand_op.

qingqing01 · 2017-10-20T10:19:51Z

paddle/operators/seq_expand_op.cc

+    }
+    PADDLE_ENFORCE(ctx->HasOutput("Out"),
+                   "Output(Out) of SeqExpandOp should not be null.");
+    ctx->SetOutputDim("Out", out_dim);


Whether the InferShape should set the LoD for output LoDTensor? Here, the LoD will be computed in the forward according the attr and input LoDs. I'm not sure wether the InferShape needs to infer all the shape info (dimension, LoD). @reyoung @jacquesqiao @QiJune

qingqing01 · 2017-10-20T10:25:54Z

paddle/operators/seq_expand_op.cc

+    repeat = 2
+then we get 1-level LoDTensor
+    Out.data = [1, 1, 2, 2, 3, 3, 4, 4]
+    Out.lod = [[0, 2, 4, 6, 8]]


These examples are good, but still hard to understand. Need some more details, since this the changes for LoD are a bit complex. For example, explain the Repeatting, it takes one instance (maybe other words) as a unit.

… seq_expand_op

Superjomn · 2017-10-25T17:07:04Z

paddle/operators/seq_expand_op.cc

+ protected:
+  void InferShape(framework::InferShapeContext* ctx) const override {
+    PADDLE_ENFORCE(ctx->HasInput("X"),
+                   "Input(X) of SeqExpandOp should not be null.");


Just PADDLE_ENFORCE_NOT_NULL(Input(X) ?
and this comment lack information, tells nothing more than the enforce code itself.

So enforce without comment is ok, or with a comment that really helps to find out the reason for its failure.

Superjomn · 2017-10-25T17:10:25Z

paddle/operators/seq_expand_op.cc

+
+Case 1:
+
+Given 2-level a LoDTensor input(X)


Make sure this op support empty sequence, if it supports, add a case because this scenario is special.

for example, Y's LoD is 1 2 2 2, that means there are 2 empty sequences.

Some instance in X should be dropped when a corresponding LoD element is empty.

Fixed by adding unitest case and comments.

Superjomn · 2017-10-25T17:14:49Z

paddle/operators/seq_expand_op.h

+};
+
+template <typename Place, typename T>
+class SeqExpandGradKernel : public framework::OpKernel<T> {


add more comments to describe the process because the code is long and hard to understand.

qingqing01 · 2017-10-26T12:07:58Z

paddle/operators/seq_expand_op.cc

+             "The element numbers of last level in input('Y') "
+             "must be equal to dims[0] of input('X').");
+    AddOutput("Out",
+              "The output of seq_expand op."


Add type for the output: (LoDTensor) The ...

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

paddle/operators/seq_expand_op.cc

+    X.dims = [4, 1]
+and input(Y)
+    Y.lod = [[0,    2,    4],
+             [0, 3, 6, 7, 8]]


Add the necessary condition? Y.lod[0][-1] == X.dims[0]

qingqing01 · 2017-10-26T12:11:45Z

paddle/operators/seq_expand_op.cc

+    X.lod = NULL
+    X.dims = [3, 1]
+and input(Y)
+    Y.lod = [[0, 2, 3, 6]]


Also add the necessary condition: len(Y.lod[0]) -1 == X.dims[0]

qingqing01 · 2017-10-26T12:11:45Z

paddle/operators/seq_expand_op.cc

+    X.lod = NULL
+    X.dims = [3, 1]
+and input(Y)
+    Y.lod = [[0, 2, 3, 6]]


Also add the necessary condition: len(Y.lod[0]) == X.dims[0]

qingqing01 · 2017-10-26T12:11:45Z

paddle/operators/seq_expand_op.cc

+    X.lod = NULL
+    X.dims = [3, 2]
+and input(Y)
+    Y.lod = [[0, 2, 3, 6]]


same as above.

qingqing01 · 2017-10-26T12:13:56Z

paddle/operators/seq_expand_op.h

+                      "The size of last lod level in Input(Y)"
+                      "must be equal to dims[0] of Input(X).");
+    out->set_lod(y->lod());
+    out->Resize(y->dims());


The dimension has been set in the InferShape, so this line can be removed.

qingqing01 · 2017-10-26T12:14:54Z

paddle/operators/seq_expand_op.h

+                      "The size of last lod level in Input(Y)"
+                      "must be equal to dims[0] of Input(X).");
+    out->set_lod(y->lod());
+    out->Resize(y->dims());


The dimension has been set in the InferShape.

qingqing01 · 2017-10-26T12:18:04Z

paddle/operators/seq_expand_op.h

+    const T* d_out_data = d_out->data<T>();
+    auto d_out_dims = d_out->dims();
+    T* d_x_data = d_x->mutable_data<T>(context.GetPlace());
+    size_t element_len = framework::product(d_out_dims) / d_out_dims[0];


remove line 71:

size_t element_len = d_out->numel() / d_out->dims()[0];

… seq_expand_op

2. Fix comments and paddle enforce check

qingqing01

LGTM.

luotao1 · 2017-10-30T04:52:24Z

paddle/operators/seq_expand_op.cc

+             "It must be a LoDTensor with k-level(k>0)."
+             "Input(X) will be expanded according to LOD of input(Y)."
+             "The element numbers of last level in input('Y') "
+             "must be equal to dims[0] of input('X').");


有的地方用input('X')，用的地方用Input(X)，是不是要统一下
下面的注释里也是一样。

luotao1 · 2017-10-30T04:53:24Z

paddle/operators/seq_expand_op.cc

+    PADDLE_ENFORCE(ctx->HasOutput("Out"));
+    PADDLE_ENFORCE(
+        ctx->HasInput("Y"),
+        "Input(Y) of SeqExpandOp should not be null while repeat == 0.");


while -> when?
repeat是从哪儿来呢？

Superjomn

LGTM

wanghaoshuang added 3 commits October 11, 2017 23:09

Add seq_expand op

901b041

1. Add unitest 2. Add SeqExpandOpKernel

fix issues

acd1aae

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

f984cba

… seq_expand_op

qingqing01 added the OpPorting label Oct 12, 2017

Superjomn mentioned this pull request Oct 13, 2017

Neural Machine Translation demo #4766

Closed

10 tasks

qingqing01 mentioned this pull request Oct 13, 2017

Review operators required by books. #4786

Closed

36 tasks

wanghaoshuang added 7 commits October 18, 2017 11:32

Refine op

23701ff

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

555ab3f

… seq_expand_op

Fix unitest

8de04be

Add backward kernel

31531ab

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

a5adffd

… seq_expand_op

Refine comments and function name

a94b3dd

1. Add more comments and exmples 2. Rename repeat_lod to expand_lod 3. Remove unused head file

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

74b283c

… seq_expand_op

wanghaoshuang requested review from luotao1, lcy-seso and QiJune October 19, 2017 09:18

QiJune reviewed Oct 20, 2017

View reviewed changes

Use stream while memory::Copy in GPU mode

00ad751

qingqing01 reviewed Oct 20, 2017

View reviewed changes

wanghaoshuang added 4 commits October 23, 2017 14:17

Modified code using LoDTensor

d697b6a

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

4e8fccf

… seq_expand_op

Rewrite sequence expand op

2961674

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

97f1b98

… seq_expand_op

qingqing01 requested a review from Superjomn October 25, 2017 08:10

Superjomn reviewed Oct 25, 2017

View reviewed changes

qingqing01 reviewed Oct 26, 2017

View reviewed changes

wanghaoshuang added 4 commits October 29, 2017 10:29

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

35e7944

… seq_expand_op

Add empty sequence case in unitest

fab6f30

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

9f32b61

… seq_expand_op

1. Add unitest for empty sequence case

8d4e2d4

2. Fix comments and paddle enforce check

qingqing01 previously approved these changes Oct 30, 2017

View reviewed changes

luotao1 reviewed Oct 30, 2017

View reviewed changes

Fix comments

84f471b

wanghaoshuang dismissed qingqing01’s stale review via 84f471b October 30, 2017 05:44

luotao1 approved these changes Oct 30, 2017

View reviewed changes

Superjomn approved these changes Oct 30, 2017

View reviewed changes

wanghaoshuang merged commit 03136f6 into PaddlePaddle:develop Oct 30, 2017

wanghaoshuang deleted the seq_expand_op branch June 1, 2018 05:41

Seq expand op #4740

Seq expand op #4740

Conversation

wanghaoshuang commented Oct 12, 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

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

Superjomn Oct 25, 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

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

Superjomn left a comment

Choose a reason for hiding this comment

wanghaoshuang commented Oct 12, 2017 •

edited

Loading

Superjomn Oct 25, 2017 •

edited

Loading