Add rank loss operator by kuke · Pull Request #4098 · PaddlePaddle/Paddle

kuke · 2017-09-14T03:13:59Z

Resolve #4065

… rank_loss_op_dev

Yancey0623 · 2017-09-15T02:15:04Z

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

+        num = 5
+        # P = {0, 1.0} or {0, 0.5, 1.0}
+        P = np.random.randint(0, 2, size=(num, num)).astype("float32")
+        Oi = np.random.random((num, num)).astype("float32")


Local variable name should be lower_with_under, https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming

Yancey0623 · 2017-09-15T02:20:14Z

paddle/operators/rank_loss_op.cc

+
+A detailed explanation about these notations can be found in
+
+[1]. Chris Burges, Tal Shaked, Erin Renshaw, et al. Learning to


Maybe we can add the link here.

JiayiFeng · 2017-09-18T18:09:43Z

paddle/operators/rank_loss_op.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("P", "The desired target values for posteriors.");
+    AddInput("Oi", "The model output for item i.");
+    AddInput("Oj", "The model output for item j.");


Please illustrate dimensions of inputs and outputs in their comments.

JiayiFeng · 2017-09-18T22:12:23Z

paddle/operators/rank_loss_op.cc

+    auto dims = ctx.Input<framework::Tensor>("P")->dims();
+    ctx.Output<framework::Tensor>(framework::GradVarName("P"))->Resize(dims);
+    ctx.Output<framework::Tensor>(framework::GradVarName("Oi"))->Resize(dims);
+    ctx.Output<framework::Tensor>(framework::GradVarName("Oj"))->Resize(dims);


Gradient Op's output(the gradient of forward op's inputs) can be nullptr, which means they are not necessary for backward. So we shall assert it is not nullptr before Resize.

See https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/cos_sim_op.cc#L146

JiayiFeng · 2017-09-18T22:31:01Z

paddle/operators/rank_loss_op.h

+    auto* oi_t = ctx.Input<framework::Tensor>("Oi");
+    auto* oj_t = ctx.Input<framework::Tensor>("Oj");
+
+    d_oi->mutable_data<T>(ctx.GetPlace());


Outputs of gradient Op may be nullptr. If so, it means that they are useless for backward and we don't need to compute them.

see https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/cos_sim_op.h#L104 for an example.

JiayiFeng · 2017-09-18T22:43:20Z

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

+from op_test import OpTest
+
+
+class TestReshapeOp(OpTest):


Why the class name is TestReshapeOp?

A typo. Fixed

JiayiFeng · 2017-09-18T22:45:56Z

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

+    def test_check_output(self):
+        self.check_output()
+
+    def test_check_grad(self):


Add some check_grad_ignore_XXX tests if posiible.
In check_grad_ignore_XXX tests, ignored variables' gradients will be set nullptr and your kernel should not compute it.

Example: https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/framework/tests/test_mul_op.py#L21

kuke

Refine this operator by following all comments. Please continue to review.

kuke · 2017-09-20T09:22:01Z

paddle/operators/rank_loss_op.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("P", "The desired target values for posteriors.");
+    AddInput("Oi", "The model output for item i.");
+    AddInput("Oj", "The model output for item j.");


kuke · 2017-09-20T09:22:07Z

paddle/operators/rank_loss_op.cc

+
+A detailed explanation about these notations can be found in
+
+[1]. Chris Burges, Tal Shaked, Erin Renshaw, et al. Learning to


kuke · 2017-09-20T09:22:14Z

paddle/operators/rank_loss_op.cc

+    auto dims = ctx.Input<framework::Tensor>("P")->dims();
+    ctx.Output<framework::Tensor>(framework::GradVarName("P"))->Resize(dims);
+    ctx.Output<framework::Tensor>(framework::GradVarName("Oi"))->Resize(dims);
+    ctx.Output<framework::Tensor>(framework::GradVarName("Oj"))->Resize(dims);


kuke · 2017-09-20T09:22:23Z

paddle/operators/rank_loss_op.h

+    auto* oi_t = ctx.Input<framework::Tensor>("Oi");
+    auto* oj_t = ctx.Input<framework::Tensor>("Oj");
+
+    d_oi->mutable_data<T>(ctx.GetPlace());


kuke · 2017-09-20T09:22:48Z

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

+from op_test import OpTest
+
+
+class TestReshapeOp(OpTest):


A typo. Fixed

kuke · 2017-09-20T09:22:54Z

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

+        num = 5
+        # P = {0, 1.0} or {0, 0.5, 1.0}
+        P = np.random.randint(0, 2, size=(num, num)).astype("float32")
+        Oi = np.random.random((num, num)).astype("float32")


kuke · 2017-09-20T09:23:19Z

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

+    def test_check_output(self):
+        self.check_output()
+
+    def test_check_grad(self):


JiayiFeng

LGTM

@lcy-seeo 's review is needed

JiayiFeng

LGTM

Yibing Liu added 2 commits September 14, 2017 11:12

add rank_loss operator

96500af

add unit test for rank_loss_op

7c423e4

qingqing01 added the OpPorting label Sep 14, 2017

Yibing Liu added 3 commits September 14, 2017 12:09

update doc information

87de31b

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

36f349e

… rank_loss_op_dev

merge conflicts

f2442be

qingqing01 requested review from JiayiFeng and Yancey0623 September 14, 2017 12:10

Yancey0623 reviewed Sep 15, 2017

View reviewed changes

Merge branch 'develop' of upstream into rank_loss_op_dev

26b393f

JiayiFeng reviewed Sep 18, 2017

View reviewed changes

Yibing Liu added 2 commits September 20, 2017 14:11

Merge branch 'develop' of upstream into rank_loss_op_dev

f2cfa32

refine rank_loss_op

ece3291

kuke commented Sep 20, 2017

View reviewed changes

kuke requested review from chengxiaohua1105 and lcy-seso and removed request for chengxiaohua1105 September 20, 2017 09:27

JiayiFeng previously approved these changes Sep 21, 2017

View reviewed changes

Yibing Liu added 2 commits September 21, 2017 11:39

change the dims of input of rank_loss_op

cf4b2db

fix a typo in rank_loss_op

1f6b909

kuke mentioned this pull request Sep 21, 2017

Add margin rank loss operator #4285

Merged

JiayiFeng approved these changes Sep 21, 2017

View reviewed changes

kuke merged commit d827359 into PaddlePaddle:develop Sep 21, 2017


		A detailed explanation about these notations can be found in

		[1]. Chris Burges, Tal Shaked, Erin Renshaw, et al. Learning to

Comments

Conversation

kuke commented Sep 14, 2017

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!

kuke 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!

JiayiFeng left a comment

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!

4 participants