Add crop op #3906

wanghaoshuang · 2017-09-06T06:50:25Z

… crop_op

dzhwinter · 2017-09-08T18:27:39Z

paddle/operators/crop_op.cc

+ AddInput("X", "The input of crop op");
+ AddInput("Y", "The input used as reference for cropping. ");
+ AddOutput("Out", "The output of crop op.");
+ AddComment(R"DOC(


This is a bad documentation, The offsets for cropping contains no information at all. Maybe describe some detail like (not necessary)

in which scene this Op will be used.

Input, Output of the Op.

the detail of this attribute, such as shape refers to the sliced tensor shape.

Thx. I will refine it.

dzhwinter · 2017-09-08T18:30:36Z

paddle/operators/crop_op.h

+ typename IndexType = Eigen::DenseIndex>
+using EigenTensor = framework::EigenTensor<T, D, MajorType, IndexType>;
+
+using Tensor = framework::Tensor;


follow the name convention of the google style click here in detail. we should not use any using in the header file.

In addition, Tensor has no template parameter, Just write

using framework::Tensor;

is okay.

dzhwinter · 2017-09-11T21:52:00Z

paddle/operators/crop_op.cc

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


I can not find the test case contains Y as input. There is only normal one with offset and shape.
Can you give a proper application case that this usage('Y' as input) is necessary? Thx.

dzhwinter · 2017-09-11T21:55:22Z

paddle/operators/crop_op.cc

+ "Shape size should be equal to dimention size of input tensor.");
+ std::vector<int64_t> tensor_shape(shape.size());
+ for (int i = 0; i < shape.size(); ++i) {
+ tensor_shape[i] = (int64_t)shape[i];


we have the interface accept the parameter of vector<int> make_ddim in ddim, do we have to convert its type?

dzhwinter · 2017-09-11T22:01:13Z

paddle/operators/crop_op.h

+ typename IndexType = Eigen::DenseIndex>
+using EigenTensor = framework::EigenTensor<T, D, MajorType, IndexType>;
+
+using Tensor = framework::Tensor;


In addition, Tensor has no template parameter, Just write

using framework::Tensor;

is okay.

dzhwinter · 2017-09-11T22:03:04Z

paddle/operators/crop_op.h

+ auto out_dims = out->dims();
+
+ auto offsets = context.op().GetAttr<std::vector<int>>("offsets");
+ PADDLE_ENFORCE_EQ(


Please move this enforce check into Infershape.
Operator's Infershape will be run before runtime, that's where real config check happens.

… crop_op Conflicts: paddle/pybind/pybind.cc

… crop_op Conflicts: paddle/pybind/pybind.cc python/paddle/v2/framework/tests/op_test_util.py

qingqing01 · 2017-09-14T12:30:32Z

paddle/operators/crop_op.cc

+ using framework::OperatorWithKernel::OperatorWithKernel;
+
+ protected:
+ void InferShape(const framework::InferShapeContext &ctx) const override {


InferShape里的检查需要全面一些，比如对输入非空的检查： https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/squared_l2_distance_op.cc#L26

Thx. Fixed.

qingqing01 · 2017-09-14T12:32:06Z

paddle/operators/crop_op.cc

+ for (int i = 0; i < shape.size(); ++i) {
+ tensor_shape[i] = (int64_t)shape[i];
+ }
+ ctx.Output<Tensor>("Out")->Resize(


LoDTensor合入后，InferShape的Output里要使用：Output<framework::LoDTensor>

qingqing01 · 2017-09-14T12:32:15Z

paddle/operators/crop_op.cc

+ ctx.Output<Tensor>("Out")->Resize(
+ paddle::framework::make_ddim(tensor_shape));
+ } else {
+ ctx.Output<Tensor>("Out")->Resize(Y->dims());


LoDTensor合入后，InferShape的Output里要使用：Output<framework::LoDTensor>

qingqing01 · 2017-09-14T12:33:32Z

paddle/operators/crop_op.cc

+ PADDLE_ENFORCE_NOT_NULL(ctx.InputVar(framework::GradVarName("Out")),
+ "Input(Out@GRAD) should not be null");
+ auto x_dims = ctx.Input<Tensor>("X")->dims();
+ auto *x_grad = ctx.Output<Tensor>(framework::GradVarName("X"));


Output使用：Output< framework ::LoDTensor>

qingqing01 · 2017-09-14T12:39:23Z

paddle/operators/crop_op.h

+ CropFunction<Place, T, 6>(context);
+ break;
+ default:
+ LOG(ERROR) << "Only ranks up to 6 supported.";


报错不要使用GLOG，使用PADDLE_ENFORCE/PADDLE_THROW:

PADDLE_THROW("Tensors with rank at most 6 are supported").

qingqing01 · 2017-09-14T12:39:54Z

paddle/operators/crop_op.h

+ CropGradFunction<Place, T, 6>(context);
+ break;
+ default:
+ LOG(ERROR) << "Only ranks up to 6 supported.";


qingqing01 · 2017-09-14T12:40:04Z

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

+ self.outputs = {'Out': self.inputs['X'][2:10, 3:11]}
+
+
+class TestCropGradOp(GradientChecker):


使用新的单测框架。

qingqing01 · 2017-09-14T12:40:13Z

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

+
+ def test_normal(self):
+ self.check_grad(
+ self.op, self.inputs, set(["X"]), "Out", max_relative_error=0.5)


max_relative_error=0.5 太大

… crop_op Conflicts: paddle/pybind/pybind.cc

… crop_op

2. Fix unitest

… crop_op

qingqing01 · 2017-09-18T11:54:01Z

paddle/operators/crop_op.cc

+ PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("X"),
+ "Input(X) of CropOp should not be null.");
+ PADDLE_ENFORCE_NOT_NULL(ctx.OutputVar("Out"),
+ "Output(Out) of CropOp should not be null.");


Put the check before line 30. 一般先check，再使用，检查正常了，再开始使用。

qingqing01 · 2017-09-18T11:55:19Z

paddle/operators/crop_op.cc

+ "Shape size should be equal to dimention size of input tensor.");
+ std::vector<int64_t> tensor_shape(shape.size());
+ for (size_t i = 0; i < shape.size(); ++i) {
+ tensor_shape[i] = (int64_t)shape[i];


static_cast< int64_t>(shape[i])

qingqing01 · 2017-09-18T11:55:49Z

paddle/operators/crop_op.cc

+ "The size of shape list should be as same as "
+ "dimension size of input X.")
+ .SetDefault(std::vector<int>());
+ }


Put the AddComment(R"DOC( )DOC") at last.

qingqing01 · 2017-09-18T11:57:50Z

paddle/operators/crop_op.cc

+
+X = [[0, 1, 2, 0, 0]
+ [0, 3, 4, 0, 0]
+ [0, 0, 0, 0, 0]]


将来Doc可能转成MarkDown，为了显示效果， line 84 - line 86加缩进吧， line 90, 94，98相同。

qingqing01 · 2017-09-18T11:58:36Z

paddle/operators/crop_op.cc

+ T *out_data = out->mutable_data<T>(context.GetPlace());
+ auto x_dims = x->dims();
+ auto out_dims = out->dims();
+ int64_t out_count = framework::product(out_dims);


out_count -> out_dims->numel()

qingqing01 · 2017-09-18T12:00:27Z

paddle/operators/crop_op.cc

+ public:
+ void Compute(const framework::ExecutionContext &context) const override {
+ auto *x = context.Input<LoDTensor>("X");
+ auto *out = context.Output<LoDTensor>("Out");


在非sequence operators里的OpKernel里实际可以不使用LoDTensor，用Tensor即可：

#4151

qingqing01 · 2017-09-18T12:01:14Z

paddle/operators/crop_op.cc

+ std::vector<int64_t> x_shape = framework::vectorize(x_dims);
+ std::vector<int64_t> out_shape = framework::vectorize(out_dims);
+
+ auto offsets = context.op().Attr<std::vector<int>>("offsets");


context.op().Attr<> -> context.Attr<>

qingqing01 · 2017-09-18T12:35:18Z

paddle/operators/crop_op.cu

+ T* out_data = out->mutable_data<T>(paddle::platform::GPUPlace());
+ auto x_dims = x->dims();
+ auto out_dims = out->dims();
+ int64_t out_count = framework::product(out_dims);


use Tensor::numel() instead of framework::product.

qingqing01 · 2017-09-18T12:36:18Z

paddle/operators/crop_op.cu

+ cudaMemcpy(x_shape_gpu, x_shape, sizeof(int64_t) * D, cudaMemcpyHostToDevice);
+ cudaMalloc((void**)&out_shape_gpu, sizeof(int64_t) * D);
+ cudaMemcpy(out_shape_gpu, out_shape, sizeof(int64_t) * D,
+ cudaMemcpyHostToDevice);


Do not use cudaMalloc and cudaMemcpy, use Tensor's function. Use Tensor as the temp variables.

reyoung · 2017-09-19T03:07:58Z

@wanghaoshuang I just push PR #4174, which will make this PR not to use const_cast. Please update.

Also, I think maybe Transform is better to implement Crop.

https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/platform/transform.h

reyoung · 2017-09-19T03:08:18Z

I will give a try and submit a PR to you.

reyoung · 2017-09-19T03:18:15Z

Sorry, I misunderstand what crop means. It seems that it is not related to Transform at all.

… crop_op

reyoung · 2017-09-19T21:33:23Z

Please reference #4205. I think Crop and Concat can be abstracted by StridedMemcpy

… crop_op

dzhwinter

LGTM++, need to modify the code based on comments slightly.

dzhwinter · 2017-09-21T01:45:46Z

paddle/operators/crop_op.cc

+ PADDLE_ENFORCE_NOT_NULL(ctx.OutputVar("Out"),
+ "Output(Out) of CropOp should not be null.");
+ auto x_dim = ctx.Input<LoDTensor>("X")->dims();
+ auto Y = ctx.Input<LoDTensor>("Y");


if auto keyword is a pointer, please use Y*

dzhwinter · 2017-09-21T02:53:27Z

paddle/operators/crop_op.cc

+ PADDLE_ENFORCE_EQ(framework::arity(x_dim), framework::arity(Y->dims()),
+ "Tensor rank of both CropOp's "
+ "inputs must be same.");
+ ctx.Output<LoDTensor>("Out")->Resize(Y->dims());


use ctx.Output<LodTensor> twice, it's better to reuse.

dzhwinter · 2017-09-21T02:55:35Z

paddle/operators/crop_op.cu

+ int block = 512;
+ int grid = (n * d + block - 1) / block;
+
+ CropKernel<


is it re-fold by the pre-commit? The format is strange, maybe it's better to add a new line character in next line.

dzhwinter · 2017-09-21T02:57:43Z

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

@@ -0,0 +1,91 @@
+import unittest


the copyrights.

好像所有单测都没copyrights。确定需要加上么？

dzhwinter

LGTM++

wanghaoshuang added 2 commits September 6, 2017 14:47

Add crop op.

06b42e9

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

96a7c70

… crop_op

qingqing01 added the OpPorting label Sep 6, 2017

qingqing01 requested review from typhoonzero and hedaoyuan September 6, 2017 08:26

Fix attr int_64 error.

f23ab48

dzhwinter reviewed Sep 8, 2017

View reviewed changes

dzhwinter reviewed Sep 11, 2017

View reviewed changes

wanghaoshuang added 2 commits September 14, 2017 01:05

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

7deddab

… crop_op Conflicts: paddle/pybind/pybind.cc

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

b299d07

… crop_op Conflicts: paddle/pybind/pybind.cc python/paddle/v2/framework/tests/op_test_util.py

qingqing01 reviewed Sep 14, 2017

View reviewed changes

wanghaoshuang added 9 commits September 15, 2017 18:12

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

a8584a9

… crop_op Conflicts: paddle/pybind/pybind.cc

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

b21aee6

… crop_op Conflicts: paddle/pybind/pybind.cc

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

e2d75bd

… crop_op

reste

57011b2

1. Implement GPUCrop kernel instead of eigen.

57a3b8b

2. Fix unitest

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

46888c3

… crop_op

Pull latest pybind.cc to crop_op

0c05ea3

Add CUDA stream when launching kernel.

5e0e455

remove op_test_util.py

8d9d537

qingqing01 reviewed Sep 18, 2017

View reviewed changes

Use Tensor as the temp variables instead of CUDA api

2c29cf1

wanghaoshuang added 4 commits September 19, 2017 13:45

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

fa4908d

… crop_op

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

a4b1abe

… crop_op

Remove const cast for device context

94fa9d1

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

ce709b7

… crop_op

wanghaoshuang added 2 commits September 21, 2017 10:12

Use stridecpy instead of CUDA kernel

68b5e5b

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

bc632df

… crop_op

dzhwinter previously approved these changes Sep 21, 2017

View reviewed changes

move stride function to ddim.h

8b6fda6

wanghaoshuang dismissed dzhwinter’s stale review via 8b6fda6 September 21, 2017 03:29

dzhwinter approved these changes Sep 21, 2017

View reviewed changes

wanghaoshuang merged commit da2aabb into PaddlePaddle:develop Sep 21, 2017

wanghaoshuang deleted the crop_op branch May 20, 2022 03:56

		self.outputs = {'Out': self.inputs['X'][2:10, 3:11]}


		class TestCropGradOp(GradientChecker):

Add crop op #3906

Add crop op #3906

Conversation

wanghaoshuang commented Sep 6, 2017 • edited Loading

dzhwinter Sep 8, 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

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 Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyoung commented Sep 19, 2017

reyoung commented Sep 19, 2017

reyoung commented Sep 19, 2017

reyoung commented Sep 19, 2017

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

dzhwinter left a comment

Choose a reason for hiding this comment

wanghaoshuang commented Sep 6, 2017 •

edited

Loading

dzhwinter Sep 8, 2017 •

edited

Loading

qingqing01 Sep 18, 2017 •

edited

Loading