Accuracy op #3907

typhoonzero · 2017-09-06T06:51:57Z

Fix #3840

… top_k_op

dzhwinter

Maybe it's better to split into small PRs. : )

dzhwinter · 2017-09-06T18:43:26Z

paddle/operators/accuracy_op.cc

+    auto *label = ctx.Input<framework::Tensor>("Label");
+
+    // label must be a vector
+    PADDLE_ENFORCE_EQ(label->dims().size(), 1);


Please put the comment into the value assertation.

PADDLE_ENFORCE_EQ(label->dims().size(), 1, "label must be a vector")

dzhwinter · 2017-09-06T18:44:19Z

paddle/operators/accuracy_op.cc

+
+    // label must be a vector
+    PADDLE_ENFORCE_EQ(label->dims().size(), 1);
+    PADDLE_ENFORCE_EQ(inference->dims()[0], label->dims()[0]);


One assertation should contain some info if it cores dump.

dzhwinter · 2017-09-06T18:47:50Z

paddle/operators/accuracy_op.h

+    auto* label = ctx.Input<Tensor>("Label");
+    auto* accuracy = ctx.Output<Tensor>("Accuracy");
+    const size_t topk = 1;
+    // static_cast<AttrType>(ctx.op_.GetAttr<AttrType>("topk"));


Please remove the comment if it is unused code.

dzhwinter · 2017-09-06T18:54:03Z

paddle/operators/accuracy_op.h

+    }
+
+    // FIXME(typhoonzero): we don't accumulate the accuracy for now.
+    *accuracy_data = static_cast<T>(num_correct) / static_cast<T>(num_samples);


why here need to cast to the type of T? I think accuracy_data always is float or double according to the precision.
Suppose that we are serving online, then the T will be fp16 for acceleration, the accuracy_data will get the wrong type.

dzhwinter · 2017-09-06T18:55:12Z

paddle/operators/accuracy_op.h

+    auto* inference = ctx.Input<Tensor>("Inference");
+    auto* label = ctx.Input<Tensor>("Label");
+    auto* accuracy = ctx.Output<Tensor>("Accuracy");
+    const size_t topk = 1;


seems topk is unused in Accuracy operator.

dzhwinter · 2017-09-06T19:03:40Z

paddle/operators/top_k_op.cc

+    // input must have >= 1d shape.
+    PADDLE_ENFORCE_GE(input->dims().size(), 1);
+    // input must have >= k columns.
+    PADDLE_ENFORCE_GE(input->dims()[input->dims().size() - 1], k);


Same with above. The user can read the enforce information without trace into the source code.

dzhwinter · 2017-09-06T19:09:57Z

paddle/operators/top_k_op.cu

+}
+
+template <typename T, int BlockSize>
+__device__ __forceinline__ void GetTopK(Pair<T> topk[], const T* src, int idx,


do we really need to put __forceinline__ explicitly? According to its document,
forceinline will be added by the nvcc compiler. To the best of my knowledge, function contains while, ifelse control flow loop, it can never be an inline function. Neither in c++ or cuda code.

dzhwinter · 2017-09-06T20:09:30Z

paddle/operators/top_k_op.h

+
+    // reshape input to a flattern matrix(like flat_inner_dims)
+    framework::DDim inputdims = input->dims();
+    const size_t row = framework::product(


There will be a flatten_to_2d interface in ddim, maybe should put a TODO here to replace in the future.

Also maybe we need different ways to flatten: flatten_to_2d_inner and flatten_to_2d_outter

dzhwinter · 2017-09-06T20:27:34Z

paddle/operators/top_k_op.h

+    X.reshape(flat2dims);
+
+    for (size_t i = 0; i < row; i++) {
+      // TODO(typhoonzero): make this more efficient


I don't think we have a better choice since the partial_sort is heap sort.
Topk is such a classic question, so I don't think the efficiency is a problem.

I'm think try not to copy the memory, since the input is const, we cannot do it currently.

reyoung · 2017-09-06T20:34:28Z

paddle/operators/accuracy_op.cc

+    AddOutput("Accuracy", "The accuracy of current batch");
+
+    AddComment(
+        R"DOC(Accuracy. It will print accuracy rate for classification.");


Bad comments here.

R"DOC(...)DOC" is just like python """...""", which R"DOC( is the left """ and )DOC" is the right """.

Actually, I had ever been confused about what does R"abc(....)abc" mean (abc can be anything, it is just a delimiter ).
I found these two docs solve my questions.

Just to share.

http://en.cppreference.com/w/cpp/language/string_literal

https://github.com/MicrosoftDocs/cpp-docs/blob/master/docs/cpp/string-and-character-literals-cpp.md

… accuracy_op

dzhwinter · 2017-09-13T02:41:24Z

paddle/operators/accuracy_op.cu

+    auto* accuracy = ctx.Output<Tensor>("Accuracy");
+    const int* inference_data = inference->data<int>();
+    const int* label_data = label->data<int>();
+    T* accuracy_data = accuracy->mutable_data<T>(ctx.GetPlace());


the accuracy_data should also be changed.

Sorry, what do you mean by "change", accuracy_data is passed to cuda kernel and assign the result in the cuda kernel.

Just the same CPU code below.

float* accuracy_data = accuracy->mutable_data<float>(ctx.GetPlace());

Here the accuracy_data type is T, which may lose precision, maybe its better to write as mutable_data<float>. And I noticed that the AccuracyDivideKernel received a parameter of float.

dzhwinter · 2017-09-13T02:46:09Z

paddle/operators/accuracy_op.h

+
+    // FIXME(typhoonzero): we don't accumulate the accuracy for now.
+    *accuracy_data =
+        static_cast<float>(num_correct) / static_cast<float>(num_samples);


Do we need an ENFORCE check num_samples is not equal to zero? when user misuse this operator, the num_samples may be zero. I'm not sure it's useful.

Done, this return 0 if num_sample==0

… accuracy_op

dzhwinter · 2017-09-13T17:09:20Z

paddle/operators/accuracy_op.cu

+    auto* accuracy = ctx.Output<Tensor>("Accuracy");
+    const int* inference_data = inference->data<int>();
+    const int* label_data = label->data<int>();
+    T* accuracy_data = accuracy->mutable_data<T>(ctx.GetPlace());


Just the same CPU code below.

float* accuracy_data = accuracy->mutable_data<float>(ctx.GetPlace());

Here the accuracy_data type is T, which may lose precision, maybe its better to write as mutable_data<float>. And I noticed that the AccuracyDivideKernel received a parameter of float.

dzhwinter · 2017-09-13T17:25:26Z

paddle/operators/accuracy_op.cu

+    size_t num_samples = inference->dims()[0];
+    size_t infer_width = inference->dims()[1];
+
+    AccuracyDivideKernel<<<1, 1>>>(num_samples, infer_width, 1, inference_data,


I'm not very familiar to cuda. It seems launched kernel in <<<1,1>>> can not utilize the capacity of cuda. This operator writes the similar one crossEntropy, but I don't know how the block=512 comes from.
@qingqing01

Well, I'm still trying to let the kernel use more threads, I'll enhance the kernel in next PR. I've got some problem writing kernel with atocimAdd.

dzhwinter

LGTM++

typhoonzero added 12 commits August 28, 2017 21:12

init add

a1348f2

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

5fb4271

… top_k_op

add topk op

5f53184

someupdate

b933d69

fix style check

95da792

add test py file

0504b5b

update top k cuda kernel

a975b2f

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

861d43e

… top_k_op

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

3aafa66

… top_k_op

follow comments

e3b78dc

remove debug print

cb99e4d

accuracy_op

99f71a8

qingqing01 added the OpPorting label Sep 6, 2017

typhoonzero added 2 commits September 6, 2017 15:11

fix casting error

68d2c5a

fix casting error

5add8bd

qingqing01 requested review from helinwang, pkuyym and dzhwinter September 6, 2017 08:26

typhoonzero added 2 commits September 6, 2017 17:10

fix casting error

fc53ed0

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

d1b39ee

… top_k_op

dzhwinter reviewed Sep 6, 2017

View reviewed changes

reyoung reviewed Sep 6, 2017

View reviewed changes

typhoonzero added 2 commits September 7, 2017 13:13

fix rename bug...

d24885f

Merge branch 'top_k_op' of https://github.com/typhoonzero/Paddle into…

49e3383

… accuracy_op

typhoonzero changed the title ~~Accuracy op~~ [WIP] Accuracy op Sep 7, 2017

typhoonzero added 4 commits September 7, 2017 15:36

make it smaller

f4fc1e7

follow comments

61e908d

update cast

343f788

update unittest

32b78b0

typhoonzero changed the title ~~[WIP] Accuracy op~~ Accuracy op Sep 13, 2017

dzhwinter reviewed Sep 13, 2017

View reviewed changes

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

33b8dd6

… accuracy_op

dzhwinter reviewed Sep 13, 2017

View reviewed changes

dzhwinter approved these changes Sep 14, 2017

View reviewed changes

typhoonzero merged commit 2d62336 into PaddlePaddle:develop Sep 14, 2017

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

add lite-hrnet_256x192 to keypoint model_zoo (PaddlePaddle#3907)

879c90b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accuracy op #3907

Accuracy op #3907

typhoonzero commented Sep 6, 2017

dzhwinter left a comment

dzhwinter Sep 6, 2017

typhoonzero Sep 13, 2017

dzhwinter Sep 6, 2017

typhoonzero Sep 13, 2017

dzhwinter Sep 6, 2017

typhoonzero Sep 13, 2017

dzhwinter Sep 6, 2017

typhoonzero Sep 13, 2017

dzhwinter Sep 6, 2017

typhoonzero Sep 13, 2017

dzhwinter Sep 6, 2017

dzhwinter Sep 6, 2017

dzhwinter Sep 6, 2017

typhoonzero Sep 7, 2017

dzhwinter Sep 6, 2017

typhoonzero Sep 7, 2017

reyoung Sep 6, 2017

lcy-seso Sep 7, 2017 •

edited

dzhwinter Sep 13, 2017

typhoonzero Sep 13, 2017

dzhwinter Sep 13, 2017

dzhwinter Sep 13, 2017 •

edited

typhoonzero Sep 14, 2017

dzhwinter Sep 13, 2017

dzhwinter Sep 13, 2017

typhoonzero Sep 14, 2017

dzhwinter left a comment

Accuracy op #3907

Accuracy op #3907

Conversation

typhoonzero commented Sep 6, 2017

dzhwinter left a 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

lcy-seso Sep 7, 2017 • edited

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

dzhwinter Sep 13, 2017 • edited

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

dzhwinter left a comment

Choose a reason for hiding this comment

lcy-seso Sep 7, 2017 •

edited

dzhwinter Sep 13, 2017 •

edited