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 Lod Reset Operator #4747

Merged
merged 3 commits into from
Nov 14, 2017
Merged

Add Lod Reset Operator #4747

merged 3 commits into from
Nov 14, 2017

Conversation

kuke
Copy link
Contributor

@kuke kuke commented Oct 12, 2017

Resolve #4719

@kuke kuke changed the title add lod_reset_op Add Lod Reset Operator Oct 12, 2017
@kuke kuke requested a review from qingqing01 October 12, 2017 04:17
qingqing01
qingqing01 reviewed Oct 20, 2017
View reviewed changes
paddle/operators/lod_reset_op.cc Outdated
AddOutput("Out", "The output tensor of lod_reset operator.");
AddAttr<std::vector<int>>("target_lod_0",
"Target level 0 LoD of "
"lod_reset operator.");
Copy link
Contributor

@qingqing01 qingqing01 Oct 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Now, the attributes are usually fixed during training, that is to say, it was passed to the network when creating the graph. I'm not sure whether the attributes can be changed during training in the future. And since the batch size and the sequence length may be varying during training, I think it's better to pass LoD as an input, not attributes. The input can be calculated by others operator.

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

qingqing01
qingqing01 reviewed Nov 9, 2017
View reviewed changes
paddle/operators/lod_reset_op.cc Outdated
LoDResetOpMaker(framework::OpProto *proto,
framework::OpAttrChecker *op_checker)
: OpProtoAndCheckerMaker(proto, op_checker) {
AddInput("X", "(LoDTensor)The input tensor of lod_reset operator.");
Copy link
Contributor

Choose a reason for hiding this comment

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

(LoDTensor)The -> (LoDTensor) The

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/operators/lod_reset_op.cc Outdated
framework::OpAttrChecker *op_checker)
: OpProtoAndCheckerMaker(proto, op_checker) {
AddInput("X", "(LoDTensor)The input tensor of lod_reset operator.");
AddInput("target_lod_in",
Copy link
Contributor

Choose a reason for hiding this comment

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

target_lod_in -> TargetLoD

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

kuke
kuke commented Nov 9, 2017
View reviewed changes
Copy link
Contributor Author

@kuke kuke left a comment

Choose a reason for hiding this comment

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

Thanks. Updated by following the comments

paddle/operators/lod_reset_op.cc Outdated
LoDResetOpMaker(framework::OpProto *proto,
framework::OpAttrChecker *op_checker)
: OpProtoAndCheckerMaker(proto, op_checker) {
AddInput("X", "(LoDTensor)The input tensor of lod_reset operator.");
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/operators/lod_reset_op.cc Outdated
framework::OpAttrChecker *op_checker)
: OpProtoAndCheckerMaker(proto, op_checker) {
AddInput("X", "(LoDTensor)The input tensor of lod_reset operator.");
AddInput("target_lod_in",
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

qingqing01
qingqing01 reviewed Nov 9, 2017
View reviewed changes
paddle/operators/lod_reset_op.h Outdated
"Target LoD should be an ascending vector.");
}

out->CopyFrom(*in, ctx.GetPlace(), ctx.device_context());
Copy link
Contributor

Choose a reason for hiding this comment

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

out->ShareDataWith(*in);

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/operators/lod_reset_op.h Outdated
d_x->mutable_data<T>(ctx.GetPlace());

auto in_dims = d_x->dims();
d_x->CopyFrom(*d_out, ctx.GetPlace(), ctx.device_context());
Copy link
Contributor

Choose a reason for hiding this comment

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

d_x->ShareDataWith(*d_out);

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/operators/lod_reset_op.cc Outdated
An example:
Given a float LoDTensor X with lod = [[0, 2, 5, 6]]

[[1, 2], [3, 4, 5], [6]]
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, the LodTensor X is saved as with shape (6, 1) in our codes:

[1,
 2,
 3,
 4,
 5,
 6]

But [[1, 2], [3, 4, 5], [6]] is easy to understand, I wonder how to add this comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

paddle/operators/lod_reset_op.h Outdated
std::vector<int> level0;
if (lod_t) {
auto* lod = lod_t->data<int>();
if (!platform::is_cpu_place(ctx.GetPlace())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (platform::is_gpu_place(ctx.GetPlace())) {
}

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

kuke
kuke commented Nov 10, 2017
View reviewed changes
Copy link
Contributor Author

@kuke kuke left a comment

Choose a reason for hiding this comment

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

Thanks

paddle/operators/lod_reset_op.cc Outdated
An example:
Given a float LoDTensor X with lod = [[0, 2, 5, 6]]

[[1, 2], [3, 4, 5], [6]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

paddle/operators/lod_reset_op.h Outdated
std::vector<int> level0;
if (lod_t) {
auto* lod = lod_t->data<int>();
if (!platform::is_cpu_place(ctx.GetPlace())) {
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/operators/lod_reset_op.h Outdated
"Target LoD should be an ascending vector.");
}

out->CopyFrom(*in, ctx.GetPlace(), ctx.device_context());
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/operators/lod_reset_op.h Outdated
d_x->mutable_data<T>(ctx.GetPlace());

auto in_dims = d_x->dims();
d_x->CopyFrom(*d_out, ctx.GetPlace(), ctx.device_context());
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

qingqing01
qingqing01 previously approved these changes Nov 10, 2017
View reviewed changes
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.

@kuke kuke closed this Nov 13, 2017
@kuke kuke reopened this Nov 13, 2017
pkuyym
pkuyym approved these changes Nov 14, 2017
View reviewed changes
Copy link
Contributor

@pkuyym pkuyym left a comment

Choose a reason for hiding this comment

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

LGTM

@kuke kuke merged commit 2f3d1c3 into PaddlePaddle:develop Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkuyym pkuyym pkuyym approved these changes

@qingqing01 qingqing01 qingqing01 left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@kuke @pkuyym @qingqing01