Add huber loss operator. #3989

pkuyym · 2017-09-09T11:03:13Z

Resolves #3923

… fix-3923-r

Superjomn · 2017-09-14T03:35:53Z

paddle/operators/huber_loss_op.cc

+    auto* x = ctx.Input<Tensor>("X");
+    auto* y = ctx.Input<Tensor>("Y");
+
+    PADDLE_ENFORCE_EQ(x->dims(), y->dims(),


PADDLE_ENFORCE_EQ means assert equality, so the comment is useless.
no comment is ok.

I have a question here, if no error information is given, does PADDLE_ENFORCE_EQ gives a meaningful error log, for example, x = 5 is not equal to y = 3.

Superjomn · 2017-09-14T03:37:05Z

paddle/operators/huber_loss_op.cc

+    // we constraint shape of X to (N, 1), may expand to (N, x, ...) if needed
+    PADDLE_ENFORCE_EQ(framework::arity(x->dims()), 2,
+                      "Tensor rank of X must be 2.");
+    PADDLE_ENFORCE_EQ(x->dims()[1], 1, "2nd dimension of X must be 1.");


the comment tell no extra information than the enforce_eq, delete the comment or make a description why it should be 1.

Agree with @Superjom

Superjomn · 2017-09-14T03:38:23Z

paddle/operators/huber_loss_op.cc

+input to (N, 1). The formulation is:
+
+L_delta(y, f(x)) = 0.5 * (y - f(x))^2                  for |y - f(x)| <= delta,
+                   delta * (|y - f(x)| - 0.5 * delta)  otherwise.


need to format the indent here?

Agree with @Superjom

Changed to Latex style.

Superjomn · 2017-09-14T03:38:58Z

paddle/operators/huber_loss_op.cc

+    auto* x_grad = ctx.Output<Tensor>(framework::GradVarName("X"));
+    auto* y_grad = ctx.Output<Tensor>(framework::GradVarName("Y"));
+
+    PADDLE_ENFORCE_NOT_NULL(x, "Input X must not be null.");


these comments lack information.

qingqing01 · 2017-09-14T13:28:54Z

paddle/operators/huber_loss_op.cc

+    PADDLE_ENFORCE_EQ(x->dims()[1], 1, "2nd dimension of X must be 1.");
+
+    ctx.Output<Tensor>("Residual")->Resize(x->dims());
+    ctx.Output<Tensor>("Out")->Resize({x->dims()[0], 1});


Output<framework::LoDTensor>

qingqing01 · 2017-09-14T13:29:36Z

paddle/operators/huber_loss_op.cc

+    auto* residual = ctx.Input<Tensor>("Residual");
+    auto* out_grad = ctx.Input<Tensor>(framework::GradVarName("Out"));
+    auto* x_grad = ctx.Output<Tensor>(framework::GradVarName("X"));
+    auto* y_grad = ctx.Output<Tensor>(framework::GradVarName("Y"));


Output<framework::LoDTensor>

qingqing01 · 2017-09-14T13:30:16Z

paddle/operators/huber_loss_op.h

+    auto* in1 = context.Input<Tensor>("Y");
+    auto* out0 = context.Output<Tensor>("Residual");
+    auto* out1 = context.Output<Tensor>("Out");
+    auto delta = static_cast<T>(context.op().Attr<AttrType>("delta"));


context.Attr<>

qingqing01 · 2017-09-14T13:30:39Z

paddle/operators/huber_loss_op.h

+using Tensor = framework::Tensor;
+template <typename T, int MajorType = Eigen::RowMajor,
+          typename IndexType = Eigen::DenseIndex>
+using EigenVector = framework::EigenVector<T, MajorType, IndexType>;


Do not use using in the header file.

qingqing01 · 2017-09-14T13:31:36Z

paddle/operators/huber_loss_op.h

+    }
+  }
+
+  bool is_x;


What is is_x ?

qingqing01 · 2017-09-14T13:33:19Z

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

+
+    def test_check_grad_ingore_y(self):
+        self.check_grad(
+            ['X'], 'Out', max_relative_error=0.5, no_grad_set=set('residual'))


The max relative error is too large.

lcy-seso · 2017-09-15T07:27:59Z

paddle/operators/huber_loss_op.cc

+    auto* x = ctx.Input<Tensor>("X");
+    auto* y = ctx.Input<Tensor>("Y");
+
+    PADDLE_ENFORCE_EQ(x->dims(), y->dims(),


I have a question here, if no error information is given, does PADDLE_ENFORCE_EQ gives a meaningful error log, for example, x = 5 is not equal to y = 3.

lcy-seso · 2017-09-15T07:31:33Z

paddle/operators/huber_loss_op.cc

+
+    PADDLE_ENFORCE_EQ(x->dims(), y->dims(),
+                      "Dimensions of X and Y must be the same.");
+    // we constraint shape of X to (N, 1), may expand to (N, x, ...) if needed


This comment has grammar errors.

we constraint shape of X to (N, 1), may expand to (N, x, ...) if needed does this means the current implementation is a simplified version, and in future, we will fix this? If so, this should be a TODO.

actually, I am not sure about the intent of this comment.

yes, PADDLE_ENFORCE_EQ will behave like Gtest's ASSERT_EQ, display details of the comparison if failed.
@lcy-seso

lcy-seso · 2017-09-15T07:32:49Z

paddle/operators/huber_loss_op.cc

+    // we constraint shape of X to (N, 1), may expand to (N, x, ...) if needed
+    PADDLE_ENFORCE_EQ(framework::arity(x->dims()), 2,
+                      "Tensor rank of X must be 2.");
+    PADDLE_ENFORCE_EQ(x->dims()[1], 1, "2nd dimension of X must be 1.");


Agree with @Superjom

lcy-seso · 2017-09-15T07:34:32Z

paddle/operators/huber_loss_op.cc

+  HuberLossOpMaker(framework::OpProto* proto,
+                   framework::OpAttrChecker* op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X", "Input value of HuberLossOp.");


Input value --> The input value，the definite article should be used in the sentence.

I think "The input tensor with shape N × K" is better. The input value is not a technically accurate description.

lcy-seso · 2017-09-15T07:34:49Z

paddle/operators/huber_loss_op.cc

+                   framework::OpAttrChecker* op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInput("X", "Input value of HuberLossOp.");
+    AddInput("Y", "Target value of HuberLossOp.");


Target value --> The output tensor with shape **

In fact, Y is also input tensor, already refined the description.

lcy-seso · 2017-09-15T07:38:08Z

paddle/operators/huber_loss_op.cc

+              "Save residual value between Y and X. "
+              "Will be reused in backward.")
+        .AsIntermediate();
+    AddOutput("Out", "Huber loss between input and target.");


Please use "the output tensor " instead, and give information of its dimension.

lcy-seso · 2017-09-15T07:39:07Z

paddle/operators/huber_loss_op.cc

+input to (N, 1). The formulation is:
+
+L_delta(y, f(x)) = 0.5 * (y - f(x))^2                  for |y - f(x)| <= delta,
+                   delta * (|y - f(x)| - 0.5 * delta)  otherwise.


Agree with @Superjom

lcy-seso · 2017-09-15T07:40:43Z

paddle/operators/huber_loss_op.cc

+    PADDLE_ENFORCE_NOT_NULL(x, "Input X must not be null.");
+    PADDLE_ENFORCE_NOT_NULL(y, "Target Y must not be null.");
+    PADDLE_ENFORCE_NOT_NULL(residual, "Residual value must not be null.");
+    PADDLE_ENFORCE_NOT_NULL(out_grad, "Out gradient must not be null.");


line 83 ~ 85, the definite article should be added.

lcy-seso · 2017-09-15T07:41:34Z

paddle/operators/huber_loss_op.cc

+    PADDLE_ENFORCE_NOT_NULL(out_grad, "Out gradient must not be null.");
+
+    PADDLE_ENFORCE_EQ(residual->dims(), x->dims(),
+                      "Dimension of X and residual value must be the same.");


The dimension

lcy-seso · 2017-09-15T07:42:09Z

paddle/operators/huber_loss_op.cc

+                      "Dimension of X and residual value must be the same.");
+    PADDLE_ENFORCE_EQ(
+        out_grad->dims(), x->dims(),
+        "Dimension of Out gradient and X must be the same (N*1).");


The dimension

qingqing01 · 2017-10-23T12:20:42Z

please update this code and merge it asap.

… fix-3923-r

lcy-seso

LGTM.

lcy-seso · 2017-10-27T03:45:55Z

paddle/operators/huber_loss_op.h

+  HOSTDEVICE T operator()(const T& val) const {
+    T abs_val = std::abs(val);
+    if (abs_val <= delta) {
+      return 0.5 * val * val;


static_cast<T>(0.5)

lcy-seso · 2017-10-27T03:47:29Z

paddle/operators/huber_loss_op.h

+    if (abs_val <= delta) {
+      return 0.5 * val * val;
+    } else {
+      return delta * (abs_val - 0.5 * delta);


static_cast<T>(0.5)

lcy-seso

LGTM.

Add huber loss operator.

1c81d57

qingqing01 added the OpPorting label Sep 11, 2017

qingqing01 requested review from Superjomn, lcy-seso and luotao1 September 11, 2017 12:27

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

df83ac4

… fix-3923-r

Superjomn reviewed Sep 14, 2017

View reviewed changes

qingqing01 reviewed Sep 14, 2017

View reviewed changes

lcy-seso requested changes Sep 15, 2017

View reviewed changes

pkuyym added 3 commits October 24, 2017 14:49

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

9a3f50d

… fix-3923-r

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

0521161

… fix-3923-r

Follow comments and adapt to new interface.

5939a17

lcy-seso previously approved these changes Oct 27, 2017

View reviewed changes

lcy-seso reviewed Oct 27, 2017

View reviewed changes

Using static_cast to make more robust.

ac5f421

pkuyym dismissed lcy-seso’s stale review via ac5f421 October 27, 2017 03:55

lcy-seso approved these changes Oct 27, 2017

View reviewed changes

pkuyym merged commit fd5199f into PaddlePaddle:develop Oct 27, 2017

Add huber loss operator. #3989

Add huber loss operator. #3989

Conversation

pkuyym commented Sep 9, 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

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 commented Oct 23, 2017

lcy-seso left a comment

Choose a reason for hiding this comment

lcy-seso Oct 27, 2017 • edited Loading

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

lcy-seso Oct 27, 2017 •

edited

Loading