-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
add new inplace api Tensor.fill_, Tensor.zero_ #33829
Conversation
Thanks for your contribution! |
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
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
Sorry to inform you that a82597a's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
python/paddle/tensor/manipulation.py
Outdated
@@ -37,6 +38,78 @@ | |||
__all__ = [] | |||
|
|||
|
|||
@dygraph_only | |||
def fill_(x, fill_value): |
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.
这里value不需要区分,比较容易理解,建议直接用value
fill_value -> value
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
namespace paddle { | ||
namespace operators { | ||
|
||
class Fill_OpMaker : public framework::OpProtoAndCheckerMaker { |
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.
这种命名方式比较奇怪,用Fill_OpMaker来区分FillOpMaker,区分度太小,
建议用FillInplaceOpMaker这种方式来区分FillOpMaker
可以确认下其他InplaceOp的写法是什么样的
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,原来的区分的方式容易跟分隔符混淆,已修改为FillInplace的命名。
python/paddle/tensor/manipulation.py
Outdated
if not (isinstance(fill_value, Variable)): | ||
fill_value = paddle.to_tensor(fill_value, dtype=x.dtype) | ||
assert (fill_value.size == 1), "var should be size 1" | ||
return core.ops.fill_inplace_(x, fill_value) |
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.
这里已经有inplace关键字了,不需要再加下划线
fill_inplace_ -> fill_inplace
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.
这里实现inplace版本沿用之前的方式,通过注册DECLARE_INPLACE_OP_INFERER的方式。
所以实际会生成正常和inplace两个版本,只是我们这个api只需要inplace版本,所以调用op还是需要下划线
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
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
import paddle.fluid as fluid | ||
fluid.set_flags({'FLAGS_eager_delete_tensor_gb': 1.0}) | ||
import paddle | ||
paddle.set_flags({'FLAGS_eager_delete_tensor_gb': 1.0}) |
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.
可以在set_flags
和get_flags
的docstring里加上对
https://www.paddlepaddle.org.cn/documentation/docs/zh/develop/guides/flags/flags_cn.html
的链接。
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
python/paddle/tensor/manipulation.py
Outdated
|
||
Args: | ||
x(Tensor): ``x`` is the Tensor we want to filled data inplace | ||
value(Scale): ``value`` is the value to fill in 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.
to be filled
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
python/paddle/tensor/manipulation.py
Outdated
|
||
import paddle | ||
|
||
tensor = paddle.to_tensor([0,1,2,3,4]) |
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.
PEP8 规范的话, 逗号后面要有空格吧。
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
@@ -37,6 +38,74 @@ | |||
__all__ = [] | |||
|
|||
|
|||
@dygraph_only | |||
def fill_(x, value): |
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.
out-place 版本我记得是paddle.full
这个为什么没用paddle.full_
,而是用了paddle.fill_
?
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.
paddle.full底层使用op fill_constant,没有反向传播,只能作为叶子节点。torch.fill_是可以反向的,所以新增了op,这样可以跟torch对齐
|
||
|
||
@dygraph_only | ||
def zero_(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.
对应的outplace版本是paddle.zeros
?
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.
是的
add fill_ backward
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
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
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
PR types
New features
PR changes
APIs
Describe
add new inplace api Tensor.fill_, Tensor.zero_