Add margin rank loss operator #4285

kuke · 2017-09-21T03:53:11Z

Resolve #4234

lcy-seso · 2017-09-25T00:28:57Z

paddle/operators/margin_rank_loss_op.cc

+    auto x2_dims = ctx.Input<framework::Tensor>("X2")->dims();
+    PADDLE_ENFORCE((label_dims == x1_dims) && (x1_dims == x2_dims) &&
+                       (label_dims.size() == 2) && (label_dims[1] == 1),
+                   "All inputs must be vector with the same size");


"All inputs must be a vector with the same size."

If the comment is a complete sentence, please add the commas at the end of the sentence.

lcy-seso · 2017-09-25T00:38:34Z

paddle/operators/margin_rank_loss_op.cc

+  MarginRankLossOpMaker(framework::OpProto *proto,
+                        framework::OpAttrChecker *op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X1", "The first variable to be ranked, row vector.");


All the comments should follow our conventions, by following the format of (type, default value) usage style. https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/name_convention.md

Personally, I don't prefer to "the first/the second", I think you can just simply write comments like this (Just for example): the input 2-D tensor with shape [N x 1], where N is the batch size. In pairwise ranking, X1 is a score for an individual item.

lcy-seso · 2017-09-25T00:44:42Z

paddle/operators/margin_rank_loss_op.cc

+                        framework::OpAttrChecker *op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X1", "The first variable to be ranked, row vector.");
+    AddInput("X2", "The second variable to be ranked, row vector.");


Please refine the comments as above.

lcy-seso · 2017-09-25T00:45:41Z

paddle/operators/margin_rank_loss_op.cc

+    AddInput("X2", "The second variable to be ranked, row vector.");
+    AddInput("Label",
+             "The label indicating X1 ranked higher than X2 "
+             "or not, row vector.");


a row vector. I think a better way is like this: A 2-D tensor with shape [N x 1]. (N has already been explained above in X1.)

Please do not forget the article.

Please add NOTE: the label can only be +1 or -1. (If I understand right.)

lcy-seso · 2017-09-25T00:50:15Z

paddle/operators/margin_rank_loss_op.cc

+
+MarginRankLoss operator measures the loss given a pair of input {`X1`, `X2`}
+and the `Label` with attribute `margin`, where `Label = 1` indicating X1 is
+ranked higher than `X2`, otherwise `Label = -1`. The loss turns out


MarginRankLoss operator measures the loss given a pair of input {X1, X2} and the Label with a margin, where Label = 1 indicating X1 is ranked higher than X2, otherwise Label = -1.
The attribute margin helps to make predictions more robust. If the negative item’s prediction exceeds that of the positive item plus a margin, then it contributes to the final loss, otherwise, does not.

lcy-seso · 2017-09-25T00:59:28Z

paddle/operators/margin_rank_loss_op.cc

+and the `Label` with attribute `margin`, where `Label = 1` indicating X1 is
+ranked higher than `X2`, otherwise `Label = -1`. The loss turns out
+
+loss(X1, X2, Label) = max(0, -Label * (X1 - X2) + margin)


From this equation, I think you should add "The label can only be +1 or -1" into comments of "Label".

lcy-seso · 2017-09-25T01:03:09Z

paddle/operators/margin_rank_loss_op.cc

+              "Intermediate tensor to indicate whether each element of "
+              "Output(Out) is activated.")
+        .AsIntermediate();
+    AddOutput("Out", "The output loss of MarginRankLoss operator");


please fix the doc by following: (type, default value) usage style.

lcy-seso · 2017-09-25T01:03:19Z

paddle/operators/margin_rank_loss_op.cc

+             "or not, row vector.");
+    AddAttr<AttrType>("margin", "Margin for MarginRankLossOp, scalar.")
+        .SetDefault(0);
+    AddOutput("Activated",


please fix the doc by following: (type, default value) usage style.

lcy-seso · 2017-09-25T01:37:45Z

paddle/operators/margin_rank_loss_op.cc

+    PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("Label"),
+                            "Input(Label) shouldn't be null");
+    PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("X1"), "Input(X1) shouldn't be null");
+    PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("X2"), "Input(X2) shouldn't be null");


Should here also check the output Var "Out" is not null?

lcy-seso · 2017-09-25T01:40:15Z

paddle/operators/margin_rank_loss_op.cc

+                       (label_dims.size() == 2) && (label_dims[1] == 1),
+                   "All inputs must be vector with the same size");
+    ctx.Output<framework::LoDTensor>("Activated")->Resize(label_dims);
+    ctx.Output<framework::LoDTensor>("Out")->Resize(label_dims);


Should here add the following codes? I am not sure, because for this operator the input X1, X2, and the output are always non-sequence. In this case, are the codes below still necessary? @qingqing01

ctx.ShareLoD("X1", /*->*/ "Out");

Maybe not necessary here

JiayiFeng · 2017-09-22T22:02:42Z

paddle/operators/margin_rank_loss_op.cc

+                       (label_dims.size() == 2) && (label_dims[1] == 1),
+                   "All inputs must be vector with the same size");
+    ctx.Output<framework::LoDTensor>("Activated")->Resize(label_dims);
+    ctx.Output<framework::LoDTensor>("Out")->Resize(label_dims);


Make sure Activated and Out are not nullptr before resize them.

JiayiFeng · 2017-09-22T22:05:09Z

paddle/operators/margin_rank_loss_op.cc

+  MarginRankLossOpMaker(framework::OpProto *proto,
+                        framework::OpAttrChecker *op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X1", "The first variable to be ranked, row vector.");


They are row vectors? not column vectors?

A mistake, corrected.

JiayiFeng · 2017-09-22T22:26:34Z

paddle/operators/margin_rank_loss_op.cc

+    AddInput("Label",
+             "The label indicating X1 ranked higher than X2 "
+             "or not, row vector.");
+    AddAttr<AttrType>("margin", "Margin for MarginRankLossOp, scalar.")


In class MarginRankLossKernel, we can see that AttrType should be consistent with T. So maybe using T directly is a better practice？

JiayiFeng · 2017-09-22T22:27:16Z

paddle/operators/margin_rank_loss_op.cc

+             "The label indicating X1 ranked higher than X2 "
+             "or not, row vector.");
+    AddAttr<AttrType>("margin", "Margin for MarginRankLossOp, scalar.")
+        .SetDefault(0);


0 should be const_cast to AttrType first.

JiayiFeng · 2017-09-22T22:33:38Z

paddle/operators/margin_rank_loss_op.h

+      return static_cast<T>(0);
+    } else {
+      return val;
+    }


return val < 0 ? static_cast<T>(0) : val;

JiayiFeng · 2017-09-25T18:00:03Z

paddle/operators/margin_rank_loss_op.h

+    } else {
+      return static_cast<T>(0);
+    }
+  }


return static_cast<T>(val > 0 ? 1 : 0);

kuke

@lcy-seso @Canpio Thanks for all comments. Updated this operator, please continue to review

kuke · 2017-09-28T04:18:48Z

paddle/operators/margin_rank_loss_op.cc

+    PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("Label"),
+                            "Input(Label) shouldn't be null");
+    PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("X1"), "Input(X1) shouldn't be null");
+    PADDLE_ENFORCE_NOT_NULL(ctx.InputVar("X2"), "Input(X2) shouldn't be null");


kuke · 2017-09-28T04:18:53Z

paddle/operators/margin_rank_loss_op.cc

+    auto x2_dims = ctx.Input<framework::Tensor>("X2")->dims();
+    PADDLE_ENFORCE((label_dims == x1_dims) && (x1_dims == x2_dims) &&
+                       (label_dims.size() == 2) && (label_dims[1] == 1),
+                   "All inputs must be vector with the same size");


kuke · 2017-09-28T04:18:59Z

paddle/operators/margin_rank_loss_op.cc

+                       (label_dims.size() == 2) && (label_dims[1] == 1),
+                   "All inputs must be vector with the same size");
+    ctx.Output<framework::LoDTensor>("Activated")->Resize(label_dims);
+    ctx.Output<framework::LoDTensor>("Out")->Resize(label_dims);


kuke · 2017-09-28T04:19:20Z

paddle/operators/margin_rank_loss_op.cc

+                       (label_dims.size() == 2) && (label_dims[1] == 1),
+                   "All inputs must be vector with the same size");
+    ctx.Output<framework::LoDTensor>("Activated")->Resize(label_dims);
+    ctx.Output<framework::LoDTensor>("Out")->Resize(label_dims);


Maybe not necessary here

kuke · 2017-09-28T04:19:40Z

paddle/operators/margin_rank_loss_op.cc

+  MarginRankLossOpMaker(framework::OpProto *proto,
+                        framework::OpAttrChecker *op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X1", "The first variable to be ranked, row vector.");


A mistake, corrected.

kuke · 2017-09-28T04:20:17Z

paddle/operators/margin_rank_loss_op.cc

+              "Intermediate tensor to indicate whether each element of "
+              "Output(Out) is activated.")
+        .AsIntermediate();
+    AddOutput("Out", "The output loss of MarginRankLoss operator");


kuke · 2017-09-28T04:20:27Z

paddle/operators/margin_rank_loss_op.cc

+
+MarginRankLoss operator measures the loss given a pair of input {`X1`, `X2`}
+and the `Label` with attribute `margin`, where `Label = 1` indicating X1 is
+ranked higher than `X2`, otherwise `Label = -1`. The loss turns out


kuke · 2017-09-28T04:20:32Z

paddle/operators/margin_rank_loss_op.cc

+and the `Label` with attribute `margin`, where `Label = 1` indicating X1 is
+ranked higher than `X2`, otherwise `Label = -1`. The loss turns out
+
+loss(X1, X2, Label) = max(0, -Label * (X1 - X2) + margin)


kuke · 2017-09-28T04:20:41Z

paddle/operators/margin_rank_loss_op.h

+      return static_cast<T>(0);
+    } else {
+      return val;
+    }


kuke · 2017-09-28T04:20:47Z

paddle/operators/margin_rank_loss_op.h

+    } else {
+      return static_cast<T>(0);
+    }
+  }


lcy-seso · 2017-09-28T06:13:28Z

Please do not forget to merge the latest develop branch.

lcy-seso · 2017-09-28T06:15:49Z

paddle/operators/margin_rank_loss_op.cc

+                       (label_dims.size() == 2) && (label_dims[1] == 1),
+                   "All inputs must be vector with the same size.");
+    auto act_t = ctx.Output<framework::LoDTensor>("Activated");
+    auto out_t = ctx.Output<framework::LoDTensor>("Out");


The InferShape interface has been changed in the latest develop branch, please do not forget to update the codes.

JiayiFeng · 2017-09-28T19:02:34Z

paddle/operators/margin_rank_loss_op.cc

+             "X2 is the score for another item to be ranked.");
+    AddInput("Label",
+             "(2-D tensor with shape [batch_size x 1]) "
+             "The label indicating X1 ranked higher than X2 or not, "


"The label indicating X1 should be ranked higher than X2 or not, "

should is not needed for the label comes from training data

JiayiFeng · 2017-09-28T19:06:06Z

paddle/operators/margin_rank_loss_op.cc

+loss(X1, X2, Label) = max(0, -Label * (X1 - X2) + margin)
+
+The attribute `margin` involved here helps make the predictions more robust.
+Only when the difference between `X1` and `X2` is greater than `margin`, it is


How to measure the difference between x1 and x2? They are two instances.

Modified the doc. Here X1 and X2 stand for the score for the two items

kuke

Update the doc

kuke · 2017-10-11T09:34:48Z

paddle/operators/margin_rank_loss_op.cc

+             "X2 is the score for another item to be ranked.");
+    AddInput("Label",
+             "(2-D tensor with shape [batch_size x 1]) "
+             "The label indicating X1 ranked higher than X2 or not, "


should is not needed for the label comes from training data

kuke · 2017-10-11T09:37:54Z

paddle/operators/margin_rank_loss_op.cc

+loss(X1, X2, Label) = max(0, -Label * (X1 - X2) + margin)
+
+The attribute `margin` involved here helps make the predictions more robust.
+Only when the difference between `X1` and `X2` is greater than `margin`, it is


Modified the doc. Here X1 and X2 stand for the score for the two items

lcy-seso

LGTM

add margin_rank_loss_op

79c2d90

qingqing01 added the OpPorting label Sep 21, 2017

Yibing Liu added 2 commits September 21, 2017 18:49

Merge branch 'develop' of upstream into margin_rank_loss_op_dev

2f12256

pass unit test for margin_rank_loss_op

6b3e9cc

kuke changed the title ~~[WIP]Add margin rank loss operator~~ Add margin rank loss operator Sep 21, 2017

kuke requested review from pkuyym, lcy-seso and JiayiFeng September 21, 2017 12:04

regulate comments in margin_rank_loss_op

756af4e

lcy-seso requested changes Sep 25, 2017

View reviewed changes

lcy-seso reviewed Sep 25, 2017

View reviewed changes

JiayiFeng reviewed Sep 25, 2017

View reviewed changes

Yibing Liu added 2 commits September 26, 2017 22:14

Merge branch 'develop' of upstream into margin_rank_loss_op_dev

dc186af

refine comments and clean code in marigin_rank_loss_op

bc2e26e

kuke commented Sep 28, 2017

View reviewed changes

lcy-seso reviewed Sep 28, 2017

View reviewed changes

Yibing Liu added 2 commits September 28, 2017 16:08

Merge branch 'develop' of upstream into margin_rank_loss_op_dev

e303897

adapt to the new infershape interface

4db50fb

JiayiFeng reviewed Sep 28, 2017

View reviewed changes

Yibing Liu added 2 commits October 11, 2017 08:26

Merge branch 'develop' of upstream into margin_rank_loss_op_dev

240adef

improve doc in margin_rank_loss_op

13b7d92

kuke commented Oct 11, 2017

View reviewed changes

fix typos in margin_rank_loss_op

989e19c

lcy-seso approved these changes Oct 11, 2017

View reviewed changes

kuke merged commit b56cbd3 into PaddlePaddle:develop Oct 11, 2017

Add margin rank loss operator #4285

Add margin rank loss operator #4285

Conversation

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

lcy-seso Sep 25, 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

kuke left a comment • 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

lcy-seso commented Sep 28, 2017

lcy-seso Sep 28, 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

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

lcy-seso left a comment

Choose a reason for hiding this comment

kuke commented Sep 21, 2017 •

edited

Loading

lcy-seso Sep 25, 2017 •

edited

Loading

kuke left a comment •

edited

Loading

lcy-seso Sep 28, 2017 •

edited

Loading