Add broadcasting support (e.g. matrix-vector) for cos sim operator. #3918

xinghai-sun · 2017-09-06T09:10:43Z

Resolve #3917

pkuyym

Almost LGTM.

pkuyym · 2017-09-11T02:51:42Z

paddle/operators/cos_sim_op.cc

+                      "Shape of Input(Out) must be [X.Dim(0), 1].");
+    auto out_grad_dims =
+        ctx.Input<Tensor>(framework::GradVarName("Out"))->dims();
+    PADDLE_ENFORCE_EQ(out_grad_dims, framework::make_ddim({x_dims[0], 1}),


You can using auto x_dims = framework::make_ddim({x_dims[0], 1}) to define x_dims first, and in the following you can just use x_dims instead.

pkuyym · 2017-09-11T02:55:00Z

paddle/operators/cos_sim_op.h

+    // compute
    auto place = context.GetEigenDevice<Place>();
-    auto xy = (x * y).sum(Eigen::array<int, 1>({1}));
    x_norm.device(place) = x.square().sum(Eigen::array<int, 1>({1})).sqrt();


You can define row_along = Eigen::array<int, 1>({1}); first.

pkuyym · 2017-09-11T02:56:52Z

paddle/operators/cos_sim_op.h

+      auto xy = (x * y).sum(Eigen::array<int, 1>({1}));
+      z.device(place) = xy / x_norm / y_norm;
+    } else {
+      Eigen::DSizes<int, 2> bcast(rows_x, 1);


better change bcast to bcast_dims?

to bcast_rows

pkuyym · 2017-09-11T03:03:08Z

paddle/operators/cos_sim_op.h

+    int rows_y = in_y->dims()[0];
+    int cols = framework::product(in_x->dims()) / rows_x;
+    auto x = EigenMatrix<T>::From(*in_x, framework::make_ddim({rows_x, cols}));
+    auto y = EigenMatrix<T>::From(*in_y, framework::make_ddim({rows_y, cols}));


you can save framework::make_ddim({rows_x, cols}) and framework::make_ddim({rows_y, cols}) to variables and reuse them in the followings.

pkuyym · 2017-09-11T03:06:10Z

paddle/operators/cos_sim_op.h

+      Eigen::DSizes<int, 2> bcast_row(rows_x, 1);
+      auto y_bcast =  y.broadcast(bcast_row);
+      auto y_snorm_bcast =
+          y_norm.square().eval().broadcast(bcast_row).eval().broadcast(bcast);


Please merge the two broadcast operations to one like broadcast({rows, cols})

JiayiFeng

Very comprehensive unit tests! Thanks!

JiayiFeng · 2017-09-11T17:35:47Z

paddle/operators/cos_sim_op.cc

+    // shape check
+    auto x_dims = ctx.Input<Tensor>("X")->dims();
+    auto y_dims = ctx.Input<Tensor>("Y")->dims();
+    PADDLE_ENFORCE_EQ(framework::arity(x_dims), framework::arity(y_dims),


The DDim has a member function size(), with returns the arity of itself.

JiayiFeng · 2017-09-11T17:40:38Z

paddle/operators/cos_sim_op.cc

+    PADDLE_ENFORCE_EQ(
+        framework::slice_ddim(x_dims, 1, framework::arity(x_dims)),
+        framework::slice_ddim(y_dims, 1, framework::arity(y_dims)),
+        "All dimensions except 1st of Input(X) and Input(Y) must be equal.");


‘... the 1st ...’ ?

JiayiFeng · 2017-09-11T17:43:06Z

paddle/operators/cos_sim_op.cc

+    auto y_dims = ctx.Input<Tensor>("Y")->dims();
+    PADDLE_ENFORCE_EQ(framework::arity(x_dims), framework::arity(y_dims),
+                      "Ranks of Input(X) and Input(Y) must be equal.");
+    PADDLE_ENFORCE_GE(framework::arity(x_dims), 2,


Why can't the rank of x be 1?

The operator assumes that the first dim of the tensor is for batch axis, so we require that the rank to be larger than 1. Otherwise，it is difficult to tell whether a vector of shape 5 is actually 5 x 1 (five samples in the batch) or 1 x 5 (one sample)?

Very reasonable! Thanks!

JiayiFeng · 2017-09-11T17:48:12Z

paddle/operators/cos_sim_op.cc

+                      "Rank of Input(X) must not be less than 2.");
+    PADDLE_ENFORCE_EQ(
+        framework::slice_ddim(x_dims, 1, framework::arity(x_dims)),
+        framework::slice_ddim(y_dims, 1, framework::arity(y_dims)),


If these two sliced dims are the same, tensors must have the same rank. The above PADDLE_ENFORCE_EQ(framework::arity(x_dims), framework::arity(y_dims), ... is unnecessary.
The infershape of backward operator has the same problem.

If we do not make sure that X and Y are of the same rank (meaning rank of Y is not less than 2), framework::slice_ddim(y_dims, 1, framework::arity(y_dims)) might throw exceptions because framework::arity(y_dims) might be smaller than 1.

JiayiFeng · 2017-09-11T17:52:25Z

paddle/operators/cos_sim_op.cc

+        framework::slice_ddim(y_dims, 1, framework::arity(y_dims)),
+        "All dimensions except 1st of Input(X) and Input(Y) must be equal.");
+    PADDLE_ENFORCE(x_dims[0] == y_dims[0] || y_dims[0] == 1,
+                   "1st dimension of Input(Y) must be equal to Input(X) or "


‘The 1st dimension...’

JiayiFeng · 2017-09-11T17:56:07Z

paddle/operators/cos_sim_op.cc

+                   "just 1 (which will be broadcasted to match Input(X)).");
+
+    // resize tensor
+    ctx.Output<Tensor>("Out")->Resize({x_dims[0], 1});


If the second dimension is just 1, why do we need it? We can squeeze it to one dimension.
The infershape of backward operator has the same problem.

Yes. That's a question we've discussed with @QiJune ---- make the output tensor to be of rank 1 or of rank 2? I prefer rank2, since when we use the output for further processing, it is very clear that it is a batch of samples of scalar instead of one single sample of vector. In a word, it reduces ambiguity. We can discuss this more.

JiayiFeng · 2017-09-11T18:00:37Z

paddle/operators/cos_sim_op.cc

+    AddInput("X", "The 1st input of cos_sim op.");
+    AddInput("Y", "The 2nd input of cos_sim op.");
    AddOutput("Out", "The output of cos_sim op.");
    AddOutput("XNorm", "Row norm of the first input.").AsIntermediate();


What is 'row norm'? Please add more instruction for this output.

JiayiFeng · 2017-09-11T18:39:45Z

paddle/operators/cos_sim_op.h

+    int rows_x = in_x->dims()[0];
+    int rows_y = in_y->dims()[0];
+    int cols = framework::product(in_x->dims()) / rows_x;
+    auto x = EigenMatrix<T>::From(*in_x, framework::make_ddim({rows_x, cols}));


EigenMatrix has a member function Reshape, which converts a paddle tensor to EigenMatrix.

static typename EigenMatrix::ConstType Reshape(const Tensor& tensor, int num_col_dims);

The EigenMatrix's first dimension(column length) will be the product of tensor's first num_col_dims dimensions. In current case, it is 1.

See https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/eigen.h#L67

JiayiFeng

LGTM. Thank you for the excellent job!

Add broadcasting support (e.g. matrix-vector) for cos sim operator.

16fddf3

xinghai-sun added the OpPorting label Sep 6, 2017

xinghai-sun requested a review from qingqing01 September 6, 2017 09:18

qingqing01 requested review from JiayiFeng, QiJune, pkuyym and qingqing01 and removed request for qingqing01 September 7, 2017 03:17

lcy-seso mentioned this pull request Sep 11, 2017

Operators 输入输出在OpProtoMaker 类中如何命名 #3996

Closed

pkuyym requested changes Sep 11, 2017

View reviewed changes

JiayiFeng reviewed Sep 11, 2017

View reviewed changes

xinghai-sun added 2 commits September 13, 2017 13:23

Update cos_sim operator by following reviewer's comments.

03ea732

Merge branch 'develop' into cos_sim_vector

965fd22

JiayiFeng approved these changes Sep 13, 2017

View reviewed changes

pkuyym approved these changes Sep 14, 2017

View reviewed changes

xinghai-sun merged commit c5972fa into PaddlePaddle:develop Sep 14, 2017

Add broadcasting support (e.g. matrix-vector) for cos sim operator. #3918

Add broadcasting support (e.g. matrix-vector) for cos sim operator. #3918

Uh oh!

Conversation

xinghai-sun commented Sep 6, 2017

Uh oh!

pkuyym left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JiayiFeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JiayiFeng left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

JiayiFeng Sep 11, 2017 •

edited

Loading