Complete smooth_l1_loss_op. #3913

pkuyym · 2017-09-06T07:37:53Z

resolves #3798

QiJune · 2017-09-07T02:12:15Z

paddle/operators/smooth_l1_loss_op.h

+
+#pragma once
+#include "paddle/framework/eigen.h"
+#include "paddle/framework/op_registry.h"


Please include this header file:

#include "paddle/platform/hostdevice.h"

QiJune · 2017-09-07T02:12:48Z

paddle/operators/smooth_l1_loss_op.h

+
+template <typename T>
+struct SmoothL1LossFoward {
+ __host__ __device__ SmoothL1LossFoward(const T& sigma2) : sigma2(sigma2) {}


Please change
__host__ __device__
to

HOSTDEVICE

QiJune · 2017-09-07T02:13:22Z

paddle/operators/smooth_l1_loss_op.h

+struct SmoothL1LossFoward {
+ __host__ __device__ SmoothL1LossFoward(const T& sigma2) : sigma2(sigma2) {}
+
+ __host__ __device__ T operator()(const T& val) const {


QiJune · 2017-09-07T02:13:31Z

paddle/operators/smooth_l1_loss_op.h

+
+template <typename T>
+struct SmoothL1LossBackward {
+ __host__ __device__ SmoothL1LossBackward(const T& sigma2) : sigma2(sigma2) {}


QiJune · 2017-09-07T02:13:38Z

paddle/operators/smooth_l1_loss_op.h

+struct SmoothL1LossBackward {
+ __host__ __device__ SmoothL1LossBackward(const T& sigma2) : sigma2(sigma2) {}
+
+ __host__ __device__ T operator()(const T& val) const {


QiJune · 2017-09-07T02:50:51Z

paddle/operators/smooth_l1_loss_op.h

+using EigenMatrix = framework::EigenMatrix<T, MajorType, IndexType>;
+
+template <typename T>
+struct SmoothL1LossFoward {


SmoothL1LossFoward -- > SmoothL1LossForward

QiJune · 2017-09-07T02:52:02Z

paddle/operators/smooth_l1_loss_op.h

+};
+
+template <typename T>
+struct SmoothL1LossBackward {


Please unify the name Backward and Grad

SmoothL1LossBackward is just a callback function used in SmoothL1LossGradKernel

pkuyym

Fix some typos and change __host__ __device__ to HOSTDEVICE

pkuyym · 2017-09-07T03:12:33Z

paddle/operators/smooth_l1_loss_op.h

+
+#pragma once
+#include "paddle/framework/eigen.h"
+#include "paddle/framework/op_registry.h"


pkuyym · 2017-09-07T03:12:44Z

paddle/operators/smooth_l1_loss_op.h

+
+template <typename T>
+struct SmoothL1LossFoward {
+ __host__ __device__ SmoothL1LossFoward(const T& sigma2) : sigma2(sigma2) {}


pkuyym · 2017-09-07T03:13:32Z

paddle/operators/smooth_l1_loss_op.h

+using EigenMatrix = framework::EigenMatrix<T, MajorType, IndexType>;
+
+template <typename T>
+struct SmoothL1LossFoward {


pkuyym · 2017-09-07T03:14:02Z

paddle/operators/smooth_l1_loss_op.h

+struct SmoothL1LossFoward {
+ __host__ __device__ SmoothL1LossFoward(const T& sigma2) : sigma2(sigma2) {}
+
+ __host__ __device__ T operator()(const T& val) const {


pkuyym · 2017-09-07T03:15:07Z

paddle/operators/smooth_l1_loss_op.h

+};
+
+template <typename T>
+struct SmoothL1LossBackward {


SmoothL1LossBackward is just a callback function used in SmoothL1LossGradKernel

pkuyym · 2017-09-07T03:15:15Z

paddle/operators/smooth_l1_loss_op.h

+
+template <typename T>
+struct SmoothL1LossBackward {
+ __host__ __device__ SmoothL1LossBackward(const T& sigma2) : sigma2(sigma2) {}


pkuyym · 2017-09-07T03:15:25Z

paddle/operators/smooth_l1_loss_op.h

+struct SmoothL1LossBackward {
+ __host__ __device__ SmoothL1LossBackward(const T& sigma2) : sigma2(sigma2) {}
+
+ __host__ __device__ T operator()(const T& val) const {


… fix-3789

QiJune · 2017-09-08T08:46:44Z

paddle/operators/smooth_l1_loss_op.h

+ Tensor paddle_weights;
+ paddle_weights.mutable_data<T>(mat_dims, context.GetPlace());
+ auto weights = EigenMatrix<T>::From(paddle_weights);
+ // initialize to 1.0


Please remove 152-159, and change to:

weights.device(place) = weight.constant(static_cast<T>(1.0));

I tried weights.device(place) = weight.constant(static_cast(1.0)); before, but it didn't work for GPU. Thanks for your suggestion, it works fine for both GPU and CPU.

QiJune · 2017-09-08T08:53:27Z

paddle/operators/smooth_l1_loss_op.h

+ SmoothL1LossBackward<T>(sigma2));
+
+ auto* out0 = context.Output<Tensor>(framework::GradVarName("X"));
+ auto* out1 = context.Output<Tensor>(framework::GradVarName("Y"));


This two lines code can move to line 172. It's better the place of declaration is close to the place of using.

Superjomn · 2017-09-08T17:38:18Z

paddle/operators/smooth_l1_loss_op.cc

+ AddOutput("diff", "Intermediate variable to cache Win*(X-Y).")
+ .AsIntermediate();
+ AddOutput("Out", "Final smooth l1 loss of inputs.");
+ AddComment(R"DOC(


put AddComment in the end?

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

paddle/operators/smooth_l1_loss_op.cc

+ PADDLE_ENFORCE_EQ(x->dims(), y->dims(),
+ "Dimensions of SmoothL1LossOp's input and target "
+ "must be same.");
+ PADDLE_ENFORCE_GE(framework::arity(x->dims()), 2,


x->dims().size()

qingqing01 · 2017-09-14T12:14:50Z

paddle/operators/smooth_l1_loss_op.cc

+ }
+
+ auto* diff = ctx.Output<framework::Tensor>("diff");
+ auto* out = ctx.Output<framework::Tensor>("Out");


auto* diff = ctx.Output<framework::LoDTensor>("diff"); auto* out = ctx.Output<framework::LoDTensor>("Out");

qingqing01 · 2017-09-14T12:16:34Z

paddle/operators/smooth_l1_loss_op.cc

+The equation is:
+loss = 0.5 * (sigma * (x - y)) ^ 2 if abs(x - y) < 1 / sigma^2
+ abs(x - y) - 0.5 / sigma^2 otherwise
+)DOC");


77行后空一行，80行前空一行，公式缩进，有可能生成MarkDown，这样有缩进好些

qingqing01 · 2017-09-14T12:16:51Z

paddle/operators/smooth_l1_loss_op.cc

+ auto* x_grad = ctx.Output<framework::Tensor>(framework::GradVarName("X"));
+ auto* y_grad = ctx.Output<framework::Tensor>(framework::GradVarName("Y"));
+
+ PADDLE_ENFORCE_GE(framework::arity(out_dims), 2,


out_dims.size()

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

paddle/operators/smooth_l1_loss_op.cc

+ AddInput("Y", "Target of SmoothL1LossOp.");
+ AddInput("InsideWeight", "Optional input to scale (X-Y).");
+ AddInput("OutsideWeight", "Optinal input to scale smooth l1 loss.");
+ AddOutput("diff", "Intermediate variable to cache Win*(X-Y).")


diff -> Diff ?

qingqing01 · 2017-09-14T12:21:34Z

paddle/operators/smooth_l1_loss_op.h

+ diff.device(place) = diff * inside_weight;
+ }
+
+ auto in_counts = framework::product(in0->dims());


auto in_counts = in0->numel();

use tensor->numel() to replace framework::product

qingqing01 · 2017-09-14T12:22:17Z

paddle/operators/smooth_l1_loss_op.h

+ Tensor paddle_errors;
+ paddle_errors.mutable_data<T>({static_cast<int>(in_counts)},
+ context.GetPlace());
+ auto errors = EigenVector<T>::Flatten(paddle_errors);


Why the name called paddle_errors ? :)

means paddle tensor errors --> ptensor_errors

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

paddle/operators/smooth_l1_loss_op.h

+ SmoothL1LossBackward<T>(sigma2));
+
+ // compute weights
+ Tensor paddle_weights;


好奇为啥名字前面加上paddle_ ?

qingqing01 · 2017-09-14T12:27:26Z

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

+ self.outputs = {'diff': diff, 'Out': loss}
+
+
+class SmoothL1LossGradOpTest(GradientChecker):


Use the new unit testing framework.

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into fix-3789

qingqing01 · 2017-09-15T08:27:18Z

paddle/operators/smooth_l1_loss_op.h

+ out1->mutable_data<T>(context.GetPlace());
+ auto place = context.GetEigenDevice<Place>();
+
+ auto sigma = static_cast<T>(context.op().Attr<AttrType>("sigma"));


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

qingqing01 · 2017-09-15T08:28:10Z

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

+ self.check_grad(
+ ['X'],
+ 'Out',
+ max_relative_error=0.08,


tune max_relative_error smaller, better to smaller than 0.02.

… fix-3789

Complete smooth_l1_loss_op.

2763f3e

qingqing01 added the OpPorting label Sep 6, 2017

qingqing01 requested review from Superjomn, xinghai-sun and Xreki September 6, 2017 08:29

Fix dimension bugs.

b7776e6

pkuyym requested a review from QiJune September 6, 2017 12:16

QiJune reviewed Sep 7, 2017

View reviewed changes

pkuyym commented Sep 7, 2017

View reviewed changes

pkuyym added 3 commits September 7, 2017 11:21

Fix typos and use HOSTDEVICE instead.

f580767

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

0728943

… fix-3789

Adapt new interface.

53ab7e7

QiJune reviewed Sep 8, 2017

View reviewed changes

Simpify the initialization for weights.

076dcb9

Superjomn reviewed Sep 8, 2017

View reviewed changes

Refine doc.

696b1f5

qingqing01 reviewed Sep 14, 2017

View reviewed changes

Using LoDTensor and adapt to new unittest.

09a13f6

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into fix-3789

qingqing01 reviewed Sep 15, 2017

View reviewed changes

pkuyym added 3 commits September 15, 2017 17:31

Tight the relative error.

3ee8765

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

12596a1

… fix-3789

Refine doc.

4e3ba65

pkuyym force-pushed the fix-3789 branch from b455b8f to 4e3ba65 Compare September 20, 2017 08:03

qingqing01 approved these changes Sep 20, 2017

View reviewed changes

pkuyym merged commit cdda0cf into PaddlePaddle:develop Sep 20, 2017

heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021

fix detach bug (PaddlePaddle#3913)

bd7838d

		self.outputs = {'diff': diff, 'Out': loss}


		class SmoothL1LossGradOpTest(GradientChecker):

Complete smooth_l1_loss_op. #3913

Complete smooth_l1_loss_op. #3913

Conversation

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

pkuyym 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

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

pkuyym left a comment •

edited

Loading