change contrib CTC to zero-indexed, in log space. sequence mask for grad #7727

szha · 2017-09-04T23:23:58Z

No description provided.

piiswrong · 2017-09-06T17:39:27Z

example/ctc/lstm_ocr.py

@@ -59,7 +59,7 @@ def gen_rand():
 def get_label(buf):
    ret = np.zeros(4)
    for i in range(len(buf)):
-        ret[i] = 1 + int(buf[i])
+        ret[i] = int(buf[i])


contrib.ctcloss has been there for a while. We should maintain backward compatibility

Making a 1-indexed API into 0-indexed API is not really backward compatible. Contrib package doesn't guarantee backward compatibility anyway.

My understanding is that zero is reserved for CTC blank label, then we shall not use zero as label (which is ensured by the old code).

With this change, alphabet_size is reserved for CTC blank label.

piiswrong · 2017-09-06T17:41:34Z

src/operator/contrib/ctc_loss.cc

@@ -35,7 +35,7 @@ ctcStatus_t compute_ctc_cost(const Tensor<cpu, 3, DType> activations,
                             void *workspace, int train) {
  int minibatch = static_cast<int>(activations.size(1));
  int alphabet_size = static_cast<int>(activations.size(2));
-  int blank_label = 0;
+  int blank_label = alphabet_size-1;


Are you sure? Shouldn't it be alphabet_size?

Yes. Alphabet size may be a misnomer, because the dimension of data is alphabet_size + 1, and this dimension is passed in as alphabet_size

On gluon/loss.py line 330, the new change specifies the following, which is inconsistent and should be corrected. Yes. It is better that alphabet_size includes blank label (with the id of alphabet_size-1).

+ input has shape `(sequence_length, batch_size, alphabet_size+1)` + Note that the last dimension with index `alphabet_size` is reserved for special + blank character.

Thanks, @zhiheng-huang. I will make an example like tf doc did to clarify.

szha · 2017-09-06T22:25:46Z

Discussed with @piiswrong offline. I will make the following changes:

get rid of the padding_mask argument that I added in my previous PR
make the default behavior of contrib CTC 1-indexed to maintain backward compatibility.
add a boolean blag to specify whether label is 0-indexed or 1-indexed. Use padding mask -1 for 0-indexed and padding mask 0 for 1-indexed for backward compatibility.
in gluon, remove padding_mask argument, and always use 0-indexed label.

piiswrong · 2017-09-08T17:06:28Z

python/mxnet/gluon/loss.py

@@ -383,5 +384,5 @@ def hybrid_forward(self, F, data, label,
                                 use_data_lengths=data_lengths is not None,
                                 use_label_lengths=label_lengths is not None,
                                 data_lengths=data_lengths, label_lengths=label_lengths,
-                                 padding_mask=self._padding_mask)
+                                 reserve_first_label=False)


reserve_first_label is a bad name.

What's your proposal?

piiswrong · 2017-09-08T23:49:28Z

src/operator/contrib/ctc_loss-inl.h

@@ -194,7 +196,7 @@ inline bool PackLabelByLength(mshadow::Tensor<xpu, 2, DType> labels,
 struct CTCLossParam : public dmlc::Parameter<CTCLossParam> {
  bool use_data_lengths;
  bool use_label_lengths;
-  dmlc::optional<int> padding_mask;
+  dmlc::optional<int> blank_label;


This doesn't need to be optional.
optional means you also accept None

piiswrong · 2017-09-10T17:41:11Z

python/mxnet/gluon/loss.py

        Its shape depends on `label_layout`. For `label_layout='TN'`, this
-        input has shape `(label_sequence_length, batch_size)`
-        When `label_lengths` is not specified, the first occurrence of `padding_mask`
+        input has shape `(label_sequence_length, batch_size)`. Padding mask of value ``-1``


this is removed right?

The parameter for padding mask is removed. The logic for padding mask value is not and should not be removed.

piiswrong · 2017-09-10T17:41:59Z

please rework documentation to reflect changes.
Add an example that explains blank label

…rad (apache#7727) * change CTC to zero-indexed, in log space. sequence mask for grad * CTC remove padding_mask, add reserve_first_label flag for blank=0/len-1 * update to blank_label enum * remove optional * update doc * dummy test

szha force-pushed the ctc_patch branch 6 times, most recently from 75cdcf0 to 89cca91 Compare September 5, 2017 19:32

piiswrong reviewed Sep 6, 2017

View reviewed changes

szha force-pushed the ctc_patch branch 2 times, most recently from d2056e5 to 8906d63 Compare September 7, 2017 06:03

szha mentioned this pull request Sep 7, 2017

CTC Loss Refactor #5559

Closed

szha force-pushed the ctc_patch branch 2 times, most recently from b36bb84 to 152b76c Compare September 7, 2017 23:20

piiswrong reviewed Sep 8, 2017

View reviewed changes

szha force-pushed the ctc_patch branch from e18cd3f to f2f6bbd Compare September 8, 2017 23:24

piiswrong reviewed Sep 8, 2017

View reviewed changes

piiswrong reviewed Sep 10, 2017

View reviewed changes

szha requested a review from mli as a code owner September 10, 2017 18:02

szha force-pushed the ctc_patch branch from 8806c33 to 7804dcd Compare September 10, 2017 18:11

szha added 5 commits September 10, 2017 22:31

change CTC to zero-indexed, in log space. sequence mask for grad

630c052

CTC remove padding_mask, add reserve_first_label flag for blank=0/len-1

e8e624f

update to blank_label enum

8f4ef91

remove optional

388be6d

update doc

44dbbdd

szha force-pushed the ctc_patch branch from 7804dcd to 44dbbdd Compare September 11, 2017 05:32

dummy test

616c84b

piiswrong merged commit 123deb0 into apache:master Sep 11, 2017

szha deleted the ctc_patch branch September 11, 2017 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change contrib CTC to zero-indexed, in log space. sequence mask for grad #7727

change contrib CTC to zero-indexed, in log space. sequence mask for grad #7727

szha commented Sep 4, 2017

piiswrong Sep 6, 2017

szha Sep 6, 2017

zhiheng-huang Sep 6, 2017

szha Sep 6, 2017

piiswrong Sep 6, 2017

szha Sep 6, 2017

zhiheng-huang Sep 6, 2017 •

edited by szha

Loading

szha Sep 6, 2017

szha commented Sep 6, 2017

piiswrong Sep 8, 2017

szha Sep 8, 2017

piiswrong Sep 8, 2017

piiswrong Sep 10, 2017

szha Sep 10, 2017

piiswrong commented Sep 10, 2017

change contrib CTC to zero-indexed, in log space. sequence mask for grad #7727

change contrib CTC to zero-indexed, in log space. sequence mask for grad #7727

Conversation

szha commented Sep 4, 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

zhiheng-huang Sep 6, 2017 • edited by szha Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szha 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

piiswrong commented Sep 10, 2017

zhiheng-huang Sep 6, 2017 •

edited by szha

Loading