Add ctc edit distance operator #5300

kuke · 2017-11-02T04:14:42Z

Resolve #4744

Xreki

About the op's name, I'd like to name it to edit_distance_op, because it is not limited to ctc.
Need to support batch_size, the inputs should be LoDTensor.
Need to call path2String to remove the uncared symbols link blank.

Xreki · 2017-11-30T06:30:05Z

paddle/operators/ctc_edit_distance_op.cc

+             "hypothesis string");
+    AddInput("X2",
+             "(2-D tensor with shape [N x 1]) The indices "
+             "for reference string.");


As I know, the inputs of this evaluator are predicted result and ground truth, please use more meaningful names.

What is the meaning of M and N? Please give more explanation.

Xreki · 2017-11-30T06:33:47Z

paddle/operators/ctc_edit_distance_op.cc

+                  "(bool, default false) Indicated whether "
+                  "normalize the Output(Out) by the length of reference "
+                  "string (X2).")
+        .SetDefault(false);


In ctc, the blank should be removed from the predicted result. In attention, the eos and sos should be removed. Maybe here, we need an attribute named uncare of type std::vector<int>, to set the elements that need to be removed.
It is not implemented in CTCErrorEvaluator.cpp, but is a real need from ocr.

I agree with that the operator needs to accept some uncared input tokens. But it would make the code dirty allowing for the CUDA kernel. How about implementing an independent op to remove the uncared tokens?

Xreki · 2017-11-30T06:35:01Z

paddle/operators/ctc_edit_distance_op.cc

+        .SetDefault(false);
+    AddOutput("Out",
+              "(2-D tensor with shape [1 x 1]) "
+              "The output distance of CTCEditDistance operator.");


batch_size should be supported in this op, the output's shape should be [batch_size, 1].

Xreki · 2017-11-30T06:42:20Z

paddle/operators/ctc_edit_distance_op.h

+    } else {
+      framework::Tensor dist_t;
+      dist_t.Resize({m + 1, n + 1});
+      dist_t.mutable_data<T>(ctx.GetPlace());


Do we need to initialize the dist_t to 0, as https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/gserver/evaluators/CTCErrorEvaluator.cpp#L90
?

It is not necessary because filling the first row and column of this matrix is enough.

Xreki · 2017-11-30T06:44:51Z

paddle/operators/ctc_edit_distance_op.h

+
+    auto m = x1_t->numel();
+    auto n = x2_t->numel();
+    T distance = 0.0;


The old codes count the substitution, deletion, insertion error, why not implement it in the op?

Usually these counts seem useless.

Xreki · 2017-11-30T06:50:51Z

paddle/operators/ctc_edit_distance_op.cc

+  void InferShape(framework::InferShapeContext *ctx) const override {
+    PADDLE_ENFORCE(ctx->HasInput("X1"), "Input(X1) shouldn't be null.");
+    PADDLE_ENFORCE(ctx->HasInput("X2"), "Input(X2) shouldn't be null.");
+    PADDLE_ENFORCE(ctx->HasOutput("Out"), "Output(Out) shouldn't be null.");


Need to check the shape of inputs.

kuke

Update by following most the comments. Thanks!

kuke · 2018-01-03T10:01:37Z

paddle/operators/ctc_edit_distance_op.cc

+  void InferShape(framework::InferShapeContext *ctx) const override {
+    PADDLE_ENFORCE(ctx->HasInput("X1"), "Input(X1) shouldn't be null.");
+    PADDLE_ENFORCE(ctx->HasInput("X2"), "Input(X2) shouldn't be null.");
+    PADDLE_ENFORCE(ctx->HasOutput("Out"), "Output(Out) shouldn't be null.");


kuke · 2018-01-03T10:01:57Z

paddle/operators/ctc_edit_distance_op.cc

+             "hypothesis string");
+    AddInput("X2",
+             "(2-D tensor with shape [N x 1]) The indices "
+             "for reference string.");


kuke · 2018-01-03T10:05:51Z

paddle/operators/ctc_edit_distance_op.cc

+                  "(bool, default false) Indicated whether "
+                  "normalize the Output(Out) by the length of reference "
+                  "string (X2).")
+        .SetDefault(false);


I agree with that the operator needs to accept some uncared input tokens. But it would make the code dirty allowing for the CUDA kernel. How about implementing an independent op to remove the uncared tokens?

kuke · 2018-01-03T10:05:59Z

paddle/operators/ctc_edit_distance_op.cc

+        .SetDefault(false);
+    AddOutput("Out",
+              "(2-D tensor with shape [1 x 1]) "
+              "The output distance of CTCEditDistance operator.");


kuke · 2018-01-03T10:07:36Z

paddle/operators/ctc_edit_distance_op.h

+
+    auto m = x1_t->numel();
+    auto n = x2_t->numel();
+    T distance = 0.0;


Usually these counts seem useless.

kuke · 2018-01-03T10:09:01Z

paddle/operators/ctc_edit_distance_op.h

+    } else {
+      framework::Tensor dist_t;
+      dist_t.Resize({m + 1, n + 1});
+      dist_t.mutable_data<T>(ctx.GetPlace());


It is not necessary because filling the first row and column of this matrix is enough.

qingqing01 · 2018-01-10T02:50:31Z

paddle/operators/edit_distance_op.cc

+             "The indices for hypothesis strings.");
+    AddInput("Refs",
+             "(2-D LoDTensor, 2nd dim. equal to 1) "
+             "The indices for reference strings.");


Should tell the users the type of Hyps and Refs is int.

2-D LoDTensor <int>

qingqing01 · 2018-01-10T02:53:56Z

paddle/operators/edit_distance_op.h

+                       n);
+        distance[num] = distance[num] / n;
+      }
+      out[num] = distance[num];


It seems, distance can be variable in T type, not a std::vector<T>.

qingqing01 · 2018-01-10T03:02:23Z

paddle/operators/edit_distance_op.cu

+        }
+        SetOutput<T><<<1, 1, 0, stream>>>(out + num, dist, m, n, normalized);
+      }
+    }


The GPU implementation may be less efficient, it may be slower than CPU implementation. The for loop in line 97 also can be paralleled. But you can not change it in this PR. We can optimize it in the future when necessary.

Yes. There should be a lot efficiency improvement for the batch input

kuke

Updated. Thanks

kuke · 2018-01-10T09:18:33Z

paddle/operators/edit_distance_op.cc

+             "The indices for hypothesis strings.");
+    AddInput("Refs",
+             "(2-D LoDTensor, 2nd dim. equal to 1) "
+             "The indices for reference strings.");


kuke · 2018-01-10T09:20:10Z

paddle/operators/edit_distance_op.cu

+        }
+        SetOutput<T><<<1, 1, 0, stream>>>(out + num, dist, m, n, normalized);
+      }
+    }


Yes. There should be a lot efficiency improvement for the batch input

kuke · 2018-01-10T09:20:47Z

paddle/operators/edit_distance_op.h

+                       n);
+        distance[num] = distance[num] / n;
+      }
+      out[num] = distance[num];


Yibing Liu added 2 commits November 2, 2017 10:28

Add edit distance operator

db69417

rename some variables in ctc_edit_distance_op

b7a4e3d

qingqing01 added the OpPorting label Nov 2, 2017

qingqing01 requested review from pkuyym and Xreki November 2, 2017 04:21

Yibing Liu added 5 commits November 27, 2017 12:56

Merge branch 'develop' of upstream into ctc_edit_distance_dev

f5681f1

add gpu kernel for ctc_edit_distance_op

6bc6ccd

clean up code in ctc_edit_distance_op

116687a

revise the doc in ctc_edit_distance_op

b82049b

Merge branch 'develop' of upstream into ctc_edit_distance_dev

c16d1ca

Xreki reviewed Nov 30, 2017

View reviewed changes

Yibing Liu added 5 commits December 27, 2017 07:51

Merge branch 'develop' of upstream into ctc_edit_distance_dev

4745a0b

Merge branch 'develop' of upstream into ctc_edit_distance_dev

36ec3e9

Rename ctc_edit_distance_op to edit_distance_op

2c1adb0

Rename inputs & format license

2e49fac

Enable batch input in edit_distance_op

0250e54

kuke commented Jan 3, 2018

View reviewed changes

qingqing01 mentioned this pull request Jan 5, 2018

The image recognition and detection model on Fluid. #7253

Closed

qingqing01 requested a review from wanghaoshuang January 10, 2018 02:19

qingqing01 reviewed Jan 10, 2018

View reviewed changes

Reuse the usable variable in edit_distance_op

f594ca4

kuke commented Jan 10, 2018

View reviewed changes

Yibing Liu added 3 commits January 10, 2018 09:26

Remove unnecessary prefix in test name of edit_distance_op

a1935b2

Merge branch 'develop' of upstream into ctc_edit_distance_dev

f3dcd00

fix ci error in edit_distance_op

fe0ef91

qingqing01 approved these changes Jan 10, 2018

View reviewed changes

kuke merged commit 861b84f into PaddlePaddle:develop Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ctc edit distance operator #5300

Add ctc edit distance operator #5300

kuke commented Nov 2, 2017 •

edited

Loading

Xreki left a comment

Xreki Nov 30, 2017

kuke Jan 3, 2018

Xreki Nov 30, 2017

kuke Jan 3, 2018

Xreki Nov 30, 2017

kuke Jan 3, 2018

Xreki Nov 30, 2017

kuke Jan 3, 2018

Xreki Nov 30, 2017

kuke Jan 3, 2018

Xreki Nov 30, 2017

kuke Jan 3, 2018

kuke left a comment

kuke Jan 3, 2018

kuke Jan 3, 2018

kuke Jan 3, 2018

kuke Jan 3, 2018

kuke Jan 3, 2018

kuke Jan 3, 2018

qingqing01 Jan 10, 2018 •

edited

Loading

kuke Jan 10, 2018

qingqing01 Jan 10, 2018

kuke Jan 10, 2018

qingqing01 Jan 10, 2018

kuke Jan 10, 2018

kuke left a comment

kuke Jan 10, 2018

kuke Jan 10, 2018

kuke Jan 10, 2018

Add ctc edit distance operator #5300

Add ctc edit distance operator #5300

Conversation

kuke commented Nov 2, 2017 • edited Loading

Xreki 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

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

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 Jan 10, 2018 • 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

Choose a reason for hiding this comment

kuke commented Nov 2, 2017 •

edited

Loading

qingqing01 Jan 10, 2018 •

edited

Loading