Add reduce op #4086

guoshengCS · 2017-09-13T17:14:24Z

Resolves #4060

luotao1 · 2017-09-15T09:09:10Z

paddle/operators/reduce_op.cc

+    ops::ReduceKernel<paddle::platform::CPUPlace, float, ops::MinFunctor>);
+REGISTER_OP_CPU_KERNEL(reduce_min_grad,
+                       ops::ReduceGradKernel<paddle::platform::CPUPlace, float,
+                                             ops::MaxOrMinGradFunctor>);


我觉得将min，max等作为attr来写，注册一个kernel，这样cc文件会更加简短精炼。

目前ReduceMinOpMaker和ReduceMaxOpMaker等基本都是重复的。现在分开写了四个kernel，写成一个后，会省将近3/4的代码。

抱歉啊，这个回复的晚了，这里参考了下tensorflow的做法 /~https://github.com/tensorflow/tensorflow/blob/216dcbf1e08c02d87774120ebd5b251c5c30c56c/tensorflow/core/kernels/reduction_ops_sum.cc#L26 ，另外pytorch中也有类似的reduce操作 /~https://github.com/pytorch/pytorch/blob/master/torch/autograd/_functions/reduce.py ，感觉可能分为多个OP在意义上更清楚一些，另外将min、max作为attr感觉会kernel部分的代码会比较长，后续可能也会加下其他的reduce操作，functor的话还有一个潜在好处是的可以复用目前的kernel不太容易复用，我也不能确定哪种会更好。多谢评论与思考建议~

luotao1 · 2017-09-15T09:11:58Z

paddle/operators/reduce_op.h

+        break;
+      case 6:
+        ReduceCompute<6>(context);
+        break;


这里的6个case是什么意思呢？

这里对应到EigenTensor的几种rank，由于EigenTensor是用的template，这里显示写了出来

luotao1 · 2017-09-15T09:14:01Z

paddle/operators/reduce_op.h

+    auto out = EigenTensor < T, D == 1 ? 1 : (D - 1) > ::From(*output, dims);
+    auto& place = context.GetEigenDevice<Place>();
+    Functor functor;
+    functor(place, x, out, reduce_dim);


如果作为attr来写，这里也不用functor，用switch来做，29-86行中一半的代码都能省出来。

Functor的话还有一个潜在好处是的可能比较容易复用，目前的kernel好像不太容易复用

luotao1 · 2017-09-15T09:15:39Z

paddle/operators/reduce_op.cu

+    ops::ReduceKernel<paddle::platform::GPUPlace, float, ops::MinFunctor>);
+REGISTER_OP_GPU_KERNEL(reduce_min_grad,
+                       ops::ReduceGradKernel<paddle::platform::GPUPlace, float,
+                                             ops::MaxOrMinGradFunctor>);


目前实现是，每个循环里调用一次eigen的gpu kernel，效率会比较慢。可以考虑单独写。

这个不是特别什么意思，可以稍具体说明下么~

gongweibao

有些代码可以省略掉

gongweibao · 2017-09-19T01:17:04Z

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

+        self.attrs = {'dim': 1}
+        self.outputs = {'Out': self.inputs['X'].mean(axis=self.attrs['dim'])}
+
+    def test_check_output(self):


定义一个父类，然后TestMeanOp(xxxx),然后test_check_out等函数就都可以重用了。

好的建议，多谢。这里reduce_max和reduce_min不进行梯度检测，几种OP中test的内容不是特别统一，感觉就先行不改了。

gongweibao · 2017-09-19T01:25:54Z

paddle/operators/reduce_op.cc

+        "(Tensor) The input tensor. Tensors with rank at most 6 are supported");
+    AddOutput("Out", "(Tensor) The result tensor.");
+    AddComment(R"DOC(
+ReduceMean operator computes the sum of input tensor along the given dimension. 


Sum,Mean,Max,Min的几个OpMaker基本都是一样的。这些代码其实可以用一个基类实现，然后子类来处理不相同的部分。
参考一下：#4139 ElementwiseOpmaker。

Done. Thanks~

gongweibao · 2017-09-19T01:31:00Z

paddle/operators/reduce_op.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput(
+        "X",
+        "(Tensor) The input tensor. Tensors with rank at most 6 are supported");


咱们的Tensor最多支持9维。这个6维是怎么来的？

这里目前参考的是crop_op中的/~https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/crop_op.h ，可能确实需要再统一下。

qingqing01 · 2017-09-14T04:38:09Z

paddle/operators/reduce_op.cc

+        "(Tensor) The input tensor. Tensors with rank at most 6 are supported");
+    AddOutput("Out", "(Tensor) The result tensor.");
+    AddComment(R"DOC(
+ReduceMean operator computes the sum of input tensor along the given dimension. 


ReduceMean -> ReduceSum

qingqing01 · 2017-09-25T02:32:48Z

paddle/operators/reduce_op.cc

+namespace operators {
+
+using framework::Tensor;
+using framework::LoDTensor;


remove this line.

qingqing01 · 2017-09-25T02:33:02Z

paddle/operators/reduce_op.cc

+      dims_vector.erase(dims_vector.begin() + dim);
+    }
+    auto out_dims = framework::make_ddim(dims_vector);
+    ctx.Output<framework::LoDTensor>("Out")->Resize(out_dims);


framework::LoDTensor -> framework::Tensor

qingqing01 · 2017-09-25T02:37:15Z

paddle/operators/reduce_op.cc

+    AddOutput("Out", "(Tensor) The result tensor.");
+    AddAttr<int>("dim",
+                 "(int, default 0) The dimension to reduce. "
+                 "Must be in the range [-rank(input), rank(input))")


Add more comments for the dim, or add some examples in the Doc.
这里-dim是倒数第几维吧？加下注释吧。

qingqing01 · 2017-09-25T03:28:42Z

paddle/operators/reduce_op.h

+    auto equals = x == y.broadcast(dim);
+    auto ones = dx.constant(1);
+    auto zeros = dx.constant(0);
+    dx.device(place) = dy.broadcast(dim) * equals.select(ones, zeros);


如果max/min值有多个，backward梯度传播的时候，这里的实现多个max值的梯度都为max的grad，而不是一些max值的梯度为0，另外，和 @guoshengCS 讨论，TF这里对多个max/min值的梯度，取了平均： /~https://github.com/tensorflow/tensorflow/blob/37f7ad75bbd2ca140d1092342eb3590d54193bc8/tensorflow/cc/gradients/math_grad.cc#L711

咱们这里的处理加下注释吧~

qingqing01 · 2017-09-25T05:09:14Z

paddle/operators/reduce_op.h

+    auto* input2 = context.Input<Tensor>(framework::GradVarName("Out"));
+    auto* output = context.Output<Tensor>(framework::GradVarName("X"));
+
+    if (output != nullptr) {


如果backward的输出只有一个的话，内部实现不用考虑output == nullptr，这种case，在/~https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/backward.cc#L67 里会返回NOP。

qingqing01 · 2017-09-25T05:11:46Z

paddle/operators/reduce_op.h

+
+ private:
+  template <size_t D>
+  void ReduceCompute(const framework::ExecutionContext& context) const {


为了和上面ReduceCompute区别，这里是否要叫做ReduceGradCompute?

qingqing01 · 2017-09-25T05:12:56Z

paddle/operators/reduce_op.h

+
+// For EigenTensor unsupported reduce
+template <typename T, typename Functor>
+class ReduceGradEigenFreeKernel : public framework::OpKernel {


这个kernel用在哪里？

Done. 这里先行删掉

… add-ReduceOp

luotao1

LGTM.

gongweibao

LGTM

QiJune mentioned this pull request Sep 13, 2017

Implement activation related operators #4071

Merged

qingqing01 added the OpPorting label Sep 14, 2017

qingqing01 requested review from Superjomn, dzhwinter, qingqing01 and hedaoyuan September 14, 2017 04:39

luotao1 reviewed Sep 15, 2017

View reviewed changes

qingqing01 requested review from QiJune and removed request for Superjomn September 18, 2017 08:33

gongweibao requested changes Sep 19, 2017

View reviewed changes

guoshengCS added 4 commits September 24, 2017 12:44

Add reduce_op

3994e91

Revise the reduce_op unit test accordingly

c8d8771

Fix reduce_op according to CI log

630273d

Refine reduce_op and follow comments

8b3bf28

guoshengCS force-pushed the add-ReduceOp branch from ebcf710 to 8b3bf28 Compare September 24, 2017 07:52

Refine reduce_op unit test and add newline at end of file

1295e5e

qingqing01 reviewed Sep 25, 2017

View reviewed changes

Refine reduce_op, follow comments and remove ReduceGradEigenFreeKernel

477a6a0

guoshengCS force-pushed the add-ReduceOp branch from 7651339 to 477a6a0 Compare September 26, 2017 07:49

guoshengCS added 3 commits September 27, 2017 11:52

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

99b8dbb

… add-ReduceOp

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

be58c63

… add-ReduceOp

Adapt reduce_op according to up-to-date dev

e33b411

luotao1 approved these changes Sep 28, 2017

View reviewed changes

gongweibao approved these changes Sep 28, 2017

View reviewed changes

guoshengCS merged commit ecef2e6 into PaddlePaddle:develop Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reduce op #4086

Add reduce op #4086

guoshengCS commented Sep 13, 2017

luotao1 Sep 15, 2017

guoshengCS Sep 18, 2017

luotao1 Sep 15, 2017

guoshengCS Sep 18, 2017

luotao1 Sep 15, 2017

guoshengCS Sep 18, 2017

luotao1 Sep 15, 2017

guoshengCS Sep 18, 2017

gongweibao left a comment

gongweibao Sep 19, 2017

guoshengCS Sep 24, 2017

gongweibao Sep 19, 2017

guoshengCS Sep 24, 2017

gongweibao Sep 19, 2017

guoshengCS Sep 24, 2017 •

edited

Loading

gongweibao Sep 28, 2017

qingqing01 Sep 14, 2017

qingqing01 Sep 25, 2017

guoshengCS Sep 25, 2017

qingqing01 Sep 25, 2017

guoshengCS Sep 25, 2017

qingqing01 Sep 25, 2017

guoshengCS Sep 25, 2017

qingqing01 Sep 25, 2017

guoshengCS Sep 25, 2017

qingqing01 Sep 25, 2017

guoshengCS Sep 25, 2017

qingqing01 Sep 25, 2017

guoshengCS Sep 25, 2017

qingqing01 Sep 25, 2017

guoshengCS Sep 25, 2017

luotao1 left a comment

gongweibao left a comment

Add reduce op #4086

Add reduce op #4086

Conversation

guoshengCS 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

gongweibao 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

guoshengCS Sep 24, 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

luotao1 left a comment

Choose a reason for hiding this comment

gongweibao left a comment

Choose a reason for hiding this comment

guoshengCS Sep 24, 2017 •

edited

Loading