Add expand operator by pkuyym · Pull Request #4061 · PaddlePaddle/Paddle

pkuyym · 2017-09-13T07:59:16Z

Resolves #4029

… fix-4029

reyoung · 2017-09-13T17:08:12Z

paddle/operators/expand_op.h

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


Do not using namespace or type alias in the header file.

reyoung · 2017-09-13T17:09:59Z

paddle/operators/expand_op.h

+  template <int Rank>
+  void Expand(const framework::ExecutionContext& context) const {
+    auto* in0 = context.Input<Tensor>("X");
+    auto expand_times = context.Attr<std::vector<int>>("expandTimes");


auto -> auto&

reyoung · 2017-09-13T17:11:14Z

paddle/operators/expand_op.h

+    auto* in0 = context.Input<Tensor>("X");
+    auto expand_times = context.Attr<std::vector<int>>("expandTimes");
+    auto* out0 = context.Output<Tensor>("Out");
+    Eigen::DSizes<int, Rank> bcast_dims;


Maybe the problem is how to cast vector<int> to Eigen::DSizes<Rank>

reyoung · 2017-09-13T22:59:24Z

Related issue #4091

guoshengCS · 2017-09-14T08:10:55Z

paddle/operators/expand_op.h

+      }
+    }
+
+    int dims = reshape_dims_vec.size() * 6 + reduce_dims_vec.size() - 7;


这里不-7的话是否ExpandBackward中 Dims / 6 + 1和 Dims % 6 + 1就可以把+1去掉了

Not really, please consider 6*1 + 6 = 12, 12/6=2 (expect 1)

jacquesqiao · 2017-09-20T00:46:48Z

paddle/operators/expand_op.cc

+    AddComment(R"DOC(
+Expand operator tiles the input by given times number. You should set times
+number for each dimension by providing attribute 'expandTimes'. Rank of input
+tensor should be in [1, 6]. Please draw an attention that size of


why Rank of input tensor should be in [1, 6]

The implementation will specialize the template due to the limitation of Eigen. I think rank 6 is big enough.

jacquesqiao · 2017-09-20T00:50:05Z

paddle/operators/expand_op.cc

+  void InferShape(const framework::InferShapeContext& ctx) const override {
+    PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("X"), "X must be initialized.");
+    std::vector<int> expand_times = Attr<std::vector<int>>("expandTimes");
+    auto* x = ctx.Input<Tensor>("X");


seems x is not used, can use

auto x_dims = ctx.Input<Tensor>("X")->dims();

instead

jacquesqiao · 2017-09-20T01:18:05Z

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

+        self.op_type = "expand"
+        self.inputs = {'X': np.random.random((12, 14)).astype("float32")}
+        self.attrs = {'expandTimes': [1, 1]}
+        output = np.tile(self.inputs['X'], (1, 1))


I think use all one as expand time is not good for test ~

Yeah, this is a corner case and I will add more robust test cases. Same as bellow.

jacquesqiao · 2017-09-20T01:18:44Z

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

+    def setUp(self):
+        self.op_type = "expand"
+        self.inputs = {'X': np.random.random((2, 4, 5)).astype("float32")}
+        self.attrs = {'expandTimes': [1, 1, 1]}


seem as above

reyoung · 2017-09-20T04:27:36Z

Maybe could use this PR #4205, too?

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into fix-4029

qingqing01 · 2017-11-03T05:31:35Z

Please update this PR and merge it asap.

… fix-4029

qingqing01 · 2017-11-09T14:21:01Z

paddle/operators/expand_op.cc

+              "dimension size of Input(X) multiplying corresponding value of "
+              "Attr(expandTimes).");
+    AddAttr<std::vector<int>>("expandTimes",
+                              "Expand times number for each dimension.");


expandTimes -> expand_times

qingqing01 · 2017-11-09T14:22:28Z

paddle/operators/expand_op.cc

+    }
+
+    ctx->SetOutputDim("Out", framework::make_ddim(out_shape));
+    ctx->ShareLoD("X", "Out");


这里应该out_shape[0] == x_dims[0]时，才能SharedLoD

qingqing01 · 2017-11-09T14:22:59Z

paddle/operators/expand_op.cc

+
+ protected:
+  void InferShape(framework::InferShapeContext* ctx) const override {
+    PADDLE_ENFORCE(ctx->HasInput("X"), "Input(X) must be initialized.");


should not be null.

Also needs to check the output.

qingqing01 · 2017-11-10T03:21:56Z

paddle/operators/expand_op.h

+    auto& expand_times = context.Attr<std::vector<int>>("expandTimes");
+    auto x_dims = in0->dims();
+    std::vector<int> reshape_dims_vec;
+    std::vector<int> reduce_dims_vec;


Please add the comments about how to compute gradients.

qingqing01 · 2017-11-10T03:23:48Z

paddle/operators/expand_op.h

+                      const std::vector<int>& reshape_dims_vec,
+                      const std::vector<int>& reduce_dims_vec) const {
+    size_t reshape_size = Dims / 6 + 1;
+    size_t reduce_size = Dims % 6 + 1;


use a constant variable to represent 6, do not use 6 directly.

qingqing01 · 2017-11-10T03:27:30Z

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

+        self.check_grad(['X'], 'Out')
+
+
+class TestExpandOpRank2_2(OpTest):


The name _1 _2 is not clear.

pkuyym added 2 commits September 13, 2017 15:57

Implemented by boost preprocessor.

ad5e7cc

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

c1215dd

… fix-4029

pkuyym requested a review from guoshengCS September 13, 2017 08:11

qingqing01 added the OpPorting label Sep 13, 2017

qingqing01 requested a review from reyoung September 13, 2017 08:12

pkuyym added 2 commits September 13, 2017 16:29

Fix typos.

f2d596d

Consider corner case.

4520afc

qingqing01 changed the title ~~Add expand tensor~~ Sep 13, 2017

reyoung reviewed Sep 13, 2017

View reviewed changes

reyoung mentioned this pull request Sep 13, 2017

Hard to manipulate Eigen::DSizes when rank of dimension is dynamic #4091

Closed

guoshengCS reviewed Sep 14, 2017

View reviewed changes

jacquesqiao reviewed Sep 20, 2017

View reviewed changes

Add more test cases and refine doc.

9367fa1

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into fix-4029

pkuyym force-pushed the fix-4029 branch from 53a09bc to 9367fa1 Compare September 20, 2017 13:21

pkuyym added 2 commits November 8, 2017 20:25

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

7be390a

… fix-4029

Adapt to new interface.

0d9ba3d

qingqing01 reviewed Nov 10, 2017

View reviewed changes

pkuyym added 2 commits November 10, 2017 15:11

Refine .cc and .h, more unit test more readable.

d04c853

Add using case.

d7e7a1d

qingqing01 approved these changes Nov 11, 2017

View reviewed changes

qingqing01 merged commit 561d634 into PaddlePaddle:develop Nov 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expand operator#4061

Add expand operator#4061
qingqing01 merged 9 commits intoPaddlePaddle:developfrom
pkuyym:fix-4029

pkuyym commented Sep 13, 2017

reyoung Sep 13, 2017

pkuyym Sep 20, 2017

reyoung Sep 13, 2017

pkuyym Sep 20, 2017

reyoung Sep 13, 2017 •

edited

Loading

reyoung commented Sep 13, 2017

guoshengCS Sep 14, 2017

pkuyym Sep 20, 2017

jacquesqiao Sep 20, 2017

pkuyym Sep 20, 2017

jacquesqiao Sep 20, 2017

pkuyym Sep 20, 2017

jacquesqiao Sep 20, 2017

pkuyym Sep 20, 2017

jacquesqiao Sep 20, 2017

reyoung commented Sep 20, 2017

qingqing01 commented Nov 3, 2017

qingqing01 Nov 9, 2017

pkuyym Nov 10, 2017

qingqing01 Nov 9, 2017

pkuyym Nov 10, 2017

qingqing01 Nov 9, 2017

pkuyym Nov 10, 2017

qingqing01 Nov 10, 2017

pkuyym Nov 10, 2017

qingqing01 Nov 10, 2017

pkuyym Nov 10, 2017

qingqing01 Nov 10, 2017

Labels

5 participants

		self.check_grad(['X'], 'Out')


		class TestExpandOpRank2_2(OpTest):

Conversation

pkuyym commented Sep 13, 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

reyoung Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

reyoung commented Sep 13, 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

reyoung commented Sep 20, 2017

qingqing01 commented Nov 3, 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

Labels

5 participants

reyoung Sep 13, 2017 •

edited

Loading