Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a nest sequence select layer. #3297

Merged
merged 10 commits into from
Aug 8, 2017
Merged

Add a nest sequence select layer. #3297

merged 10 commits into from
Aug 8, 2017

Conversation

lcy-seso
Copy link
Contributor

@lcy-seso lcy-seso commented Aug 7, 2017

  • Add a sub_nested_seq_layer.
  • sub_nested_seq_layer accepts two inputs: the first one is a nested sequence; the second one is a set of selected indices in the nested sequence.
  • sub_nest_seq_layer trims the first nested sequence input according to the selected indices to form a new output.
  • This layer will be used in beam training.

@lcy-seso lcy-seso requested a review from luotao1 August 7, 2017 05:36
luotao1
luotao1 requested changes Aug 7, 2017
View reviewed changes
paddle/gserver/layers/SubNestedSequenceLayer.cpp Outdated
return true;
}

void SubNestedSequenceLayer::reorganizeSeqInfo(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reorganizeSeqInfo这个感觉是一个比较通用的函数，把ICpuGpuVectorPtr seqStartPos和ICpuGpuVectorPtr subSeqStartPos变成了vector<vector<int>>形式，可以放到Argument.cpp中。不过可以在下一个PR中加。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

python/paddle/trainer_config_helpers/layers.py Outdated
The example usage is:

.. code-block:: python
sub_nest_seq = sub_nested_seq_layer(input=[data, selected_indices])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6109行的格式不对。应该空一行，再缩进四格。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

paddle/gserver/layers/SubNestedSequenceLayer.cpp Outdated
const ICpuGpuVectorPtr subSeqStartPos);
void calSelectedCols(const MatrixPtr selectedIndices,
const std::vector<std::vector<int>> inputSeqInfo);
void buildOutputSeqInfo();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

34,36,38行三个函数缺乏注释

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

paddle/gserver/layers/SubNestedSequenceLayer.cpp Outdated
outSubSeqStartInfo_.push_back(outSubSeqStartInfo_.back() + subSeqLen);
}
outSeqStartInfo_.push_back(outSubSeqStartInfo_.back());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildOutputSeqInfo可以和calSelectedCols合并成一个函数么？这样outSeqStartInfo_和outSubSeqStartInfo_就可以作为函数内的一个临时变量了。
主要原因是：buildOutputSeqInfo没有任何的参数，从forward的代码中看不出修改了什么变量。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

paddle/gserver/layers/SubNestedSequenceLayer.cpp
selIdsCpu_->copyFrom(*selectedIndices);
} else {
selIdsCpu_ = selectedIndices;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

132-150行需要移入calSelectedCols函数么，selIdsCpu也是一个临时变量，除了在calSelectedCols里面用，其他地方没有用到。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

paddle/gserver/layers/SubNestedSequenceLayer.cpp
for (size_t i = 0; i < seqNum; ++i) {
for (size_t j = 0; j < beamSize; ++j) {
if (selectedIndices->getElement(i, j) == -1.) break;
int selSubSeqIdx = selectedIndices->getElement(i, j);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

95行挪到96行后面：不用访问两次selectedIndices了。
if (selSubSeqIdx == -1) break;
-1后面为什么有一个.呢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里比较的是一个float。

@lcy-seso lcy-seso force-pushed the add_nest_sequence_select branch 2 times, most recently from 1d48dc0 to c192eb8 Compare August 7, 2017 09:54
lcy-seso
lcy-seso commented Aug 7, 2017
View reviewed changes
Copy link
Contributor Author

@lcy-seso lcy-seso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow comments.

paddle/gserver/layers/SubNestedSequenceLayer.cpp
for (size_t i = 0; i < seqNum; ++i) {
for (size_t j = 0; j < beamSize; ++j) {
if (selectedIndices->getElement(i, j) == -1.) break;
int selSubSeqIdx = selectedIndices->getElement(i, j);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里比较的是一个float。

paddle/gserver/layers/SubNestedSequenceLayer.cpp Outdated
outSubSeqStartInfo_.push_back(outSubSeqStartInfo_.back() + subSeqLen);
}
outSeqStartInfo_.push_back(outSubSeqStartInfo_.back());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

paddle/gserver/layers/SubNestedSequenceLayer.cpp
selIdsCpu_->copyFrom(*selectedIndices);
} else {
selIdsCpu_ = selectedIndices;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@lcy-seso lcy-seso changed the title Add nest sequence select Add a nest sequence select layer. Aug 7, 2017
luotao1
luotao1 approved these changes Aug 8, 2017
View reviewed changes
Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM。有空可以改下注释。

paddle/gserver/layers/SubNestedSequenceLayer.cpp
*
* ths output is saved to private member rowIndice_;
* [0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,
* 16,17,18,19,20,21,22,23,24,25,26,27]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

55行的示例不太好，rowIndice是连续的输出，有更好的示例么？

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的，这几个layer 都是为了做 beam training 添加的，一定会整体联调，再完善一遍注释。

@lcy-seso lcy-seso merged commit d9f97b0 into PaddlePaddle:develop Aug 8, 2017
@lcy-seso lcy-seso deleted the add_nest_sequence_select branch August 8, 2017 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luotao1 luotao1 luotao1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lcy-seso @luotao1