-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Assign Operator. #5531
Assign Operator. #5531
Conversation
Out=X, when type in [LoDTensor/SelectedRows/LoDTensorArray]
paddle/operators/assign_op.cc
Outdated
|
||
void operator()(const framework::SelectedRows &rows) const { | ||
auto &out_rows = *out_->GetMutable<framework::SelectedRows>(); | ||
*out_rows.mutable_rows() = rows.rows(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use set_rows
interface /~https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/selected_rows.h#L45 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
paddle/operators/assign_op.cc
Outdated
auto &out_rows = *out_->GetMutable<framework::SelectedRows>(); | ||
*out_rows.mutable_rows() = rows.rows(); | ||
auto &t = rows.value(); | ||
out_rows.mutable_value()->CopyFrom(t, t.place(), dev_ctx_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether needs to set height_ for out_rows
? /~https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/selected_rows.h#L59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
paddle/operators/assign_op.cc
Outdated
return; | ||
} | ||
auto *out = scope.FindVar(Output("Out")); | ||
PADDLE_ENFORCE(out != nullptr, "Must set output if x is set"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must set output if x is set ->
The Output(Out) should not be null if the Input(X) is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
paddle/operators/assign_op.cc
Outdated
: OpProtoAndCheckerMaker(proto, op_checker) { | ||
AddInput( | ||
"X", | ||
"The input variable. Could be LoDTensor/SelectedRows/LoDTensorArray.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" (LoDTensor, SelectedRows or LoDTensorArray) The input variable could be LoDTensor, SelectedRows or LoDTensorArray. "
The most operators' doc follow format: /~https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/gru_op.cc#L73
() specify the type, then explain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
paddle/operators/assign_op.cc
Outdated
"X", | ||
"The input variable. Could be LoDTensor/SelectedRows/LoDTensorArray.") | ||
.AsDispensable(); | ||
AddOutput("Out", "The output variable. Its type will be set as same as X"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" (LoDTensor, SelectedRows or LoDTensorArray) The type of output is the same as input X."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
paddle/operators/assign_op.cc
Outdated
AddOutput("Out", "The output variable. Its type will be set as same as X"); | ||
AddComment(R"DOC(Assign Operator | ||
|
||
Out = X, when type in [LoDTensor/SelectedRows/LoDTensorArray] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tell the user how to process other types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
class TestAccuracyOp(op_test.OpTest): | ||
def setUp(self): | ||
self.op_type = "assign" | ||
x = numpy.random.random(size=(100, 10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, the input type is LoDTensor. Whether needs to add the unit test to others types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other types of variable are not exposed to Python well. It will be tested during if-else operator's unit tests.
It will be added in the following PRs
import unittest | ||
|
||
|
||
class TestAccuracyOp(op_test.OpTest): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestAccuracyOp --> TestAssignOp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
paddle/operators/assign_op.cc
Outdated
|
||
void operator()(const framework::SelectedRows &rows) const { | ||
auto &out_rows = *out_->GetMutable<framework::SelectedRows>(); | ||
*out_rows.mutable_rows() = rows.rows(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to
out_rows.set_height(rows.height());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
16a7f91
to
15ee123
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Out=X, when type in [LoDTensor/SelectedRows/LoDTensorArray]