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.

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

fix_sequence_conv_op #5130

Merged
merged 4 commits into from
Oct 30, 2017
Merged

Conversation

chengduoZH
Copy link
Contributor

@chengduoZH chengduoZH commented Oct 26, 2017

refine SequenceConvOp
continue #4814

@chengduoZH chengduoZH changed the title follow comments fix_sequence_conv_op Oct 26, 2017
@@ -128,25 +128,25 @@ class SequenceConvOpMaker : public framework::OpProtoAndCheckerMaker {
"this LoDTensor is a matrix with shape (T, D), where, T is the "
"total time steps in this mini-batch, D is the output feature size.");

AddAttr<bool>("padding_trainable",
AddAttr<bool>("paddingTrainable",
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 120 - 123, the shape for Filter is not right.

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

AddAttr<int>("context_stride",
"(int, default 1) the context_stride of SequenceConvOp "
AddAttr<int>("contextStride",
"(int, default 1) the contextStride of SequenceConvOp "
"represents the step length of convolution. "
Copy link
Contributor

Choose a reason for hiding this comment

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

step length convolution -> stride length of convolution kernel.

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

AddAttr<int>("context_start",
"(int, default 0) the context_start of SequenceConvOp "
AddAttr<int>("contextStart",
"(int, default 0) the contextStart of SequenceConvOp "
"represents the beginning of the convolution of the number of "
"rows of sequence, which can be negative.")
Copy link
Contributor

Choose a reason for hiding this comment

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

More explanations:

The negative number means to pad contextStart time-steps of zeros or learnable parameters at the beginning of each instance. The positive number means to skip contextStart time-steps of each instance.

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

AddAttr<int>("context_length",
"(int, default 3) the context_length of SequenceConvOp is the "
AddAttr<int>("contextLength",
"(int, default 3) the contextLength of SequenceConvOp is the "
"height of the convolution kernel.")
.SetDefault(3)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is an argument the users must set, so please do not set the default value.

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

@chengduoZH
Copy link
Contributor Author

chengduoZH commented Oct 30, 2017

From the realization of function, using StridedCopy in context_project is also appropriate. But from the efficiency analysis, the efficiency of using StridedCopy is not high.
For example, the length of sequence is 20, context_length is 3, the realization of StridedCopy is equivalent to copying 20*3 times, and for col_matrix, the address of each time replication is discontinuous.

Copy link
Contributor

@qingqing01 qingqing01 left a comment

Choose a reason for hiding this comment

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

LGTM.

@chengduoZH chengduoZH merged commit 0f9858a into PaddlePaddle:develop Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants