Add a sequence slice layer #3367

lcy-seso · 2017-08-09T14:14:36Z

add a seq_slice_layer which will return one or several sub-sequences from the input sequence layer given start and end indices.

If only start indices are given, and end indices are set to None, this layer slices the input sequence from the given start indices to its end.
If only end indices are given and start indices are set to None, this layer slices the input sequence from its beginning to the given end indices.
If start and end indices are both given, they should have the same number of elements.

luotao1 · 2017-08-18T04:59:47Z

paddle/gserver/layers/KmaxSeqScoreLayer.cpp

+  // (with -1 as a special token). Storing indices information in real-typed
+  // Matrix leads to converting real to int. This is very dangerous if a user
+  // fills this matrix himself, invalid data may occur.
+  // The selected indices should be stored in an int-typed matrix.


101行matrices是复数。

real type, int type。

104行Matrix小写

real-typed 是构词法，等于形容词，这个也可以吧？不过这一段注释重新完善修改过。

luotao1 · 2017-08-18T05:00:43Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+  // (with -1 as a special token). Storing indices information in real-typed
+  // Matrix leads to converting real to int. This is very dangerous if a user
+  // fills this matrix himself, invalid data may occur.
+  // The selected indices should be stored in an int-typed matrix.


这段注释和KmaxSeqScoreLayer.cpp里的那段一样，保留一个就够了吧。

这些注释确实是一样的，多个文件都需统一改一下，但彼此又是独立的。
这里有一个real强制转int，比较危险，我建议还是留着。

luotao1 · 2017-08-18T05:01:38Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+void SequenceSliceLayer::checkInputs() {
+  const Argument& inputSeq = getInput(0);
+  CHECK(inputSeq.hasSeq()) << "The first input of sequence slic layer "
+                           << "must be a sequence.";


slic->slice笔误

luotao1 · 2017-08-18T05:02:41Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+  CHECK_EQ(static_cast<size_t>(indices1->getHeight()),
+           inputSeq.hasSubseq() ? inputSeq.getNumSubSequences()
+                                : inputSeq.getNumSequences())
+      << "Height of the second input should be equal to number of sequence "


The second input height should be equal to the number of sequences

The second input height 这个也有语法问题吧，应该用of 或者 's 来表示所有格。

luotao1 · 2017-08-18T05:04:57Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+      } else {
+        startIdsOnCpu_ = nullptr;
+        endIdsOnCpu_ = getInputValue(1);
+      }


91行-97行：

startIdsOnCpu_ = config_.select_first() ? getInputValue(1) : nullptr; endIdsOnCpu_ = config_.select_first() ? nullptr : getInputValue(1);

这样改的话，同一个条件需要判断两次条件，但关系不大。。。done.

luotao1 · 2017-08-18T05:10:58Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+        if (starts) {
+          if (starts->getElement(rowIdx, k) == -1.) break;
+        } else if (ends->getElement(rowIdx, k) == -1.)
+          break;


156-159行，是判断两个break的条件，等价于?：

if (starts && starts->getElement(rowIdx, k) == -1.) break; if (ends && ends->getElement(rowIdx, k) == -1.) break;

改成短路逻辑了，done。

会引入一次额外的逻辑判断，不过影响不大。。。

luotao1 · 2017-08-18T05:15:33Z

paddle/gserver/layers/SubNestedSequenceLayer.cpp

+  // (with -1 as a special token). Storing indices information in real-typed
+  // Matrix leads to converting real to int. This is very dangerous if a user
+  // fills this matrix himself, invalid data may occur.
+  // The selected indices should be stored in an int-typed matrix.


这段注释和上面的也重复了

这里还是需要保存这一段注释，因为这几个文件是各自独立的。对注释的语法做了修改。

luotao1 · 2017-08-18T05:21:55Z

python/paddle/trainer_config_helpers/layers.py

+          number of elements.
+
+    If start or end indices contains more than one elements, the input sequence
+    will be sliced for multiple times.


这里最好给例子，特别来说明start和end有多余1个元素的情况下，应该怎么输入，输出是什么

luotao1 · 2017-08-18T05:26:34Z

python/paddle/trainer/config_parser.py

+            if isinstance(starts, list):
+                assert len(starts) == 1, (
+                    'the start indices for sequence slice layer cannot '
+                    'be a list having more than one element.')


我看layer.py里面写 If start or end indices contains more than one elements。
那这里又说只能是1个？

这里是检测 starts 里面只给了一个layer，而这个layer 本身的 Argument.value 里面可以写多个要去裁切的位点。

luotao1 · 2017-08-18T05:27:08Z

python/paddle/trainer/config_parser.py

+            if starts is not None:
+                self.config.select_first = True
+            else:
+                self.config.select_first = False


2720-2723行
self.config.select_first = True if starts else False

luotao1 · 2017-08-18T05:36:51Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+    rowIndice_->copyFrom(selectedRows_.data(), selectedRows_.size());
+  } else {
+    rowIndice_ =
+        IVector::create(selectedRows_.data(), selectedRows_.size(), useGpu_);


180-185可以直接，cpu和gpu都相同吧：

rowIndice_ = IVector::create(selectedRows_.size(), useGpu_); rowIndice_->copyFrom(selectedRows_.data(), selectedRows_.size());

这里是想CPU下面避免一次拷贝。如果改成一样，CPU下需要进行一次不必要的拷贝。

luotao1 · 2017-08-18T05:45:24Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+  MatrixPtr inputSeqGrad = getInputGrad(0);
+  MatrixPtr outputGrad = getOutputGrad();
+
+  outputGrad->addToRows(*inputSeqGrad, *rowIndice_);


这三行可以合并成一行：
getOutputGrad()->addToRows(*getInputGrad(0), *rowIndice_);

lcy-seso

follow comments.

lcy-seso · 2017-08-23T05:19:51Z

paddle/gserver/layers/KmaxSeqScoreLayer.cpp

+  // (with -1 as a special token). Storing indices information in real-typed
+  // Matrix leads to converting real to int. This is very dangerous if a user
+  // fills this matrix himself, invalid data may occur.
+  // The selected indices should be stored in an int-typed matrix.


real-typed 是构词法，等于形容词，这个也可以吧？不过这一段注释重新完善修改过。

lcy-seso · 2017-08-23T05:20:08Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+void SequenceSliceLayer::checkInputs() {
+  const Argument& inputSeq = getInput(0);
+  CHECK(inputSeq.hasSeq()) << "The first input of sequence slic layer "
+                           << "must be a sequence.";


lcy-seso · 2017-08-23T05:20:34Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+  CHECK_EQ(static_cast<size_t>(indices1->getHeight()),
+           inputSeq.hasSubseq() ? inputSeq.getNumSubSequences()
+                                : inputSeq.getNumSequences())
+      << "Height of the second input should be equal to number of sequence "


The second input height 这个也有语法问题吧，应该用of 或者 's 来表示所有格。

lcy-seso · 2017-08-23T05:21:30Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+      } else {
+        startIdsOnCpu_ = nullptr;
+        endIdsOnCpu_ = getInputValue(1);
+      }


这样改的话，同一个条件需要判断两次条件，但关系不大。。。done.

lcy-seso · 2017-08-23T05:33:22Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+                           false /* trans */,
+                           false /* useGpu */);
+    endIdsOnCpu_->copyFrom(*indices2);
+  }


不是一样，91 - 97 是 cpu下，105 - 138 行在gpu下会执行到。把cpu下的从这个函数中移除了。

lcy-seso · 2017-08-23T05:55:00Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+        if (starts) {
+          if (starts->getElement(rowIdx, k) == -1.) break;
+        } else if (ends->getElement(rowIdx, k) == -1.)
+          break;


改成短路逻辑了，done。

lcy-seso · 2017-08-23T06:00:53Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+    rowIndice_->copyFrom(selectedRows_.data(), selectedRows_.size());
+  } else {
+    rowIndice_ =
+        IVector::create(selectedRows_.data(), selectedRows_.size(), useGpu_);


这里是想CPU下面避免一次拷贝。如果改成一样，CPU下需要进行一次不必要的拷贝。

lcy-seso · 2017-08-23T06:01:00Z

paddle/gserver/layers/SequenceSliceLayer.cpp

+  MatrixPtr inputSeqGrad = getInputGrad(0);
+  MatrixPtr outputGrad = getOutputGrad();
+
+  outputGrad->addToRows(*inputSeqGrad, *rowIndice_);


lcy-seso · 2017-08-23T06:01:30Z

paddle/gserver/layers/SubNestedSequenceLayer.cpp

+  // (with -1 as a special token). Storing indices information in real-typed
+  // Matrix leads to converting real to int. This is very dangerous if a user
+  // fills this matrix himself, invalid data may occur.
+  // The selected indices should be stored in an int-typed matrix.


这里还是需要保存这一段注释，因为这几个文件是各自独立的。对注释的语法做了修改。

lcy-seso · 2017-08-23T06:02:26Z

python/paddle/trainer/config_parser.py

+            if isinstance(starts, list):
+                assert len(starts) == 1, (
+                    'the start indices for sequence slice layer cannot '
+                    'be a list having more than one element.')


这里是检测 starts 里面只给了一个layer，而这个layer 本身的 Argument.value 里面可以写多个要去裁切的位点。

luotao1

LGTM

lcy-seso added 4 commits August 7, 2017 20:38

add config helper for sequence slice layer.

84627bb

add unittest.

2988a58

Merge branch 'develop' into add_sequence_slice_layer

34ff752

add implementations.

26bc5b1

lcy-seso changed the title ~~Add sequence slice layer~~ Add a sequence slice layer Aug 9, 2017

lcy-seso requested a review from luotao1 August 9, 2017 14:22

lcy-seso force-pushed the add_sequence_slice_layer branch from ebcb685 to b6b8c8e Compare August 10, 2017 01:36

Merge branch 'develop' into add_sequence_slice_layer

7ff689f

lcy-seso force-pushed the add_sequence_slice_layer branch 2 times, most recently from f5d69ca to 9e6e0a0 Compare August 10, 2017 05:14

fix unittest error.

b97f020

lcy-seso force-pushed the add_sequence_slice_layer branch from 9e6e0a0 to b97f020 Compare August 11, 2017 06:17

Merge branch 'develop' into add_sequence_slice_layer

b7359ee

luotao1 reviewed Aug 18, 2017

View reviewed changes

lcy-seso added 2 commits August 23, 2017 14:07

follow comments.

82e4fab

Merge branch 'develop' into add_sequence_slice_layer

bc6bae8

lcy-seso commented Aug 23, 2017

View reviewed changes

fix a bug.

377401f

lcy-seso force-pushed the add_sequence_slice_layer branch from 6bc93b3 to 377401f Compare August 23, 2017 06:30

lcy-seso mentioned this pull request Aug 23, 2017

add a cross_entropy_over_beam layer. #3639

Merged

luotao1 approved these changes Aug 23, 2017

View reviewed changes

lcy-seso merged commit ab6b3c4 into PaddlePaddle:develop Aug 24, 2017

lcy-seso deleted the add_sequence_slice_layer branch August 24, 2017 03:21

Add a sequence slice layer #3367

Add a sequence slice layer #3367

Conversation

lcy-seso commented Aug 9, 2017 • 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

lcy-seso 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

luotao1 left a comment

Choose a reason for hiding this comment

lcy-seso commented Aug 9, 2017 •

edited

Loading