Add reshape operator #3949

kuke · 2017-09-07T04:38:38Z

Resolve #4009

dzhwinter · 2017-09-07T21:34:12Z

paddle/operators/reshape_op.cc

+  void InferShape(const framework::InferShapeContext &ctx) const override {
+    auto *in = ctx.Input<framework::Tensor>("X");
+    auto shape = ctx.Attr<std::vector<int>>("shape");
+    PADDLE_ENFORCE_EQ((unsigned)shape.size(), in->dims().size(),


unsigned long please. the size returned by ddim is ssize_t, which is 8byte width.

Removed this unreasonable line.

dzhwinter · 2017-09-07T21:42:04Z

paddle/operators/reshape_op.h

+namespace paddle {
+namespace operators {
+
+using Tensor = framework::Tensor;


do not use using in the header file, according to the google style.

dzhwinter · 2017-09-07T21:46:19Z

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

+
+
+class ReshapeGradOpTest(GradientChecker):
+    def test_normal(self):


now can use the function setUp to generate forward operator.
please take a look at this case

dzhwinter · 2017-09-07T21:47:44Z

paddle/operators/reshape_op.h

+    out->mutable_data<T>(ctx.GetPlace());
+
+    auto shape = ctx.Attr<std::vector<int>>("shape");
+    std::vector<int64_t> tmp;


Please do not mix use of int64_t and size_t, which is ugly for users who read our code.

… reshape_op_dev

kuke

Refine this PR by following the review comments. Please continue to review.

kuke · 2017-09-11T07:11:51Z

paddle/operators/reshape_op.cc

+  void InferShape(const framework::InferShapeContext &ctx) const override {
+    auto *in = ctx.Input<framework::Tensor>("X");
+    auto shape = ctx.Attr<std::vector<int>>("shape");
+    PADDLE_ENFORCE_EQ((unsigned)shape.size(), in->dims().size(),


Removed this unreasonable line.

kuke · 2017-09-11T07:12:08Z

paddle/operators/reshape_op.h

+namespace paddle {
+namespace operators {
+
+using Tensor = framework::Tensor;


kuke · 2017-09-11T07:12:18Z

paddle/operators/reshape_op.h

+    out->mutable_data<T>(ctx.GetPlace());
+
+    auto shape = ctx.Attr<std::vector<int>>("shape");
+    std::vector<int64_t> tmp;


kuke · 2017-09-11T07:12:26Z

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

+
+
+class ReshapeGradOpTest(GradientChecker):
+    def test_normal(self):


dzhwinter

Seems LGTM.
Maybe @qingqing01 have more comment.

… reshape_op_dev

qingqing01 · 2017-09-12T02:35:45Z

paddle/operators/reshape_op.cc

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


Need to check non empty for Input('X'), like : /~https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/squared_l2_distance_op.cc#L26

qingqing01 · 2017-09-12T02:58:29Z

paddle/operators/reshape_op.cc

+      } else {
+        capacity *= dim;
+      }
+    }


int64_t capacity = 1; for (auto dim : shape) { PADDLE_ENFORCE(dim > 0, "Each dimension of shape must be positive."); capacity *= dim; }

or use std::accumulate :

int64_t capacity = std::accumulate(shape.begin(), shape.end(), 1, std::multiplies<int>());

qingqing01 · 2017-09-12T03:00:01Z

paddle/operators/reshape_op.cc

+    int64_t in_size = framework::product(in->dims());
+    PADDLE_ENFORCE_EQ(capacity, in_size,
+                      "The size of Input(X) mismatches with Attr(shape).");
+    ctx.Output<framework::Tensor>("Out")->Resize(in->dims());


Why not Resize(shape)? The dims of output is not shape ?

qingqing01 · 2017-09-12T03:01:33Z

paddle/operators/reshape_op.cc

+    AddComment(R"DOC(Reshape operator
+
+Reshape Input(X) into the shape specified by Attr(shape).
+)DOC");


Need more comments : /~https://github.com/PaddlePaddle/Paddle/pull/3765/files#diff-6e4a3431c20e09cd11b1b56451b5754eR52

doc要求： #3885 (comment)

qingqing01 · 2017-09-12T03:03:19Z

paddle/operators/reshape_op.cc

+      : OperatorWithKernel(type, inputs, outputs, attrs) {}
+
+ protected:
+  void InferShape(const framework::InferShapeContext &ctx) const override {


Need to check nonempty for the inputs.

kuke

updated, please continue to review @qingqing01

kuke · 2017-09-12T05:17:07Z

paddle/operators/reshape_op.cc

+      : OperatorWithKernel(type, inputs, outputs, attrs) {}
+
+ protected:
+  void InferShape(const framework::InferShapeContext &ctx) const override {


kuke · 2017-09-12T05:17:53Z

paddle/operators/reshape_op.cc

+    int64_t in_size = framework::product(in->dims());
+    PADDLE_ENFORCE_EQ(capacity, in_size,
+                      "The size of Input(X) mismatches with Attr(shape).");
+    ctx.Output<framework::Tensor>("Out")->Resize(in->dims());


kuke · 2017-09-12T05:17:58Z

paddle/operators/reshape_op.cc

+      } else {
+        capacity *= dim;
+      }
+    }


kuke · 2017-09-12T05:18:06Z

paddle/operators/reshape_op.cc

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


kuke · 2017-09-12T05:18:17Z

paddle/operators/reshape_op.cc

+    AddComment(R"DOC(Reshape operator
+
+Reshape Input(X) into the shape specified by Attr(shape).
+)DOC");


add reshape operator

12eaa22

qingqing01 added the OpPorting label Sep 7, 2017

pass unit test

899c7d6

dzhwinter reviewed Sep 7, 2017

View reviewed changes

qingqing01 requested a review from Superjomn September 9, 2017 02:28

Yibing Liu added 3 commits September 10, 2017 03:56

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

02da0d1

… reshape_op_dev

refine reshape operator

dd64349

merge conflicts

477d92b

kuke commented Sep 11, 2017

View reviewed changes

remove unused code in test

7ae72f7

kuke requested review from qingqing01 and reyoung September 11, 2017 08:11

kuke changed the title ~~[WIP]Add reshape operator~~ Add reshape operator Sep 11, 2017

dzhwinter previously approved these changes Sep 11, 2017

View reviewed changes

Yibing Liu added 2 commits September 11, 2017 19:42

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

31cbb34

… reshape_op_dev

adapt to the new test framework

dd92649

kuke dismissed dzhwinter’s stale review via dd92649 September 12, 2017 03:09

qingqing01 reviewed Sep 12, 2017

View reviewed changes

follow comments to cleanup code

0289a00

kuke commented Sep 12, 2017

View reviewed changes

fix a typo

5915138

qingqing01 approved these changes Sep 12, 2017

View reviewed changes

kuke merged commit 4137cb0 into PaddlePaddle:develop Sep 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reshape operator #3949

Add reshape operator #3949

kuke commented Sep 7, 2017 •

edited

Loading

dzhwinter Sep 7, 2017

kuke Sep 11, 2017

dzhwinter Sep 7, 2017

kuke Sep 11, 2017

dzhwinter Sep 7, 2017

kuke Sep 11, 2017

dzhwinter Sep 7, 2017

kuke Sep 11, 2017

kuke left a comment

kuke Sep 11, 2017

kuke Sep 11, 2017

kuke Sep 11, 2017

kuke Sep 11, 2017

dzhwinter left a comment

qingqing01 Sep 12, 2017

kuke Sep 12, 2017

qingqing01 Sep 12, 2017

kuke Sep 12, 2017

qingqing01 Sep 12, 2017

kuke Sep 12, 2017

qingqing01 Sep 12, 2017

kuke Sep 12, 2017

qingqing01 Sep 12, 2017

kuke Sep 12, 2017

kuke left a comment

kuke Sep 12, 2017

kuke Sep 12, 2017

kuke Sep 12, 2017

kuke Sep 12, 2017

kuke Sep 12, 2017



		class ReshapeGradOpTest(GradientChecker):
		def test_normal(self):

Add reshape operator #3949

Add reshape operator #3949

Conversation

kuke commented Sep 7, 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

kuke 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

dzhwinter 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

kuke 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

kuke commented Sep 7, 2017 •

edited

Loading