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

[phi::DenseTensor] Replace Tensor with phi::DenseTensor in .cc files #48525

Closed

Conversation

Liyulingyue
Copy link
Contributor

@Liyulingyue Liyulingyue commented Nov 29, 2022

@paddle-bot
Copy link

paddle-bot bot commented Nov 29, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels Nov 29, 2022
@Liyulingyue Liyulingyue changed the title Replace Tensor with phi::DenseTensor in .cc files [phi::DenseTensor] Replace Tensor with phi::DenseTensor in .cc files Nov 30, 2022
@@ -302,9 +303,9 @@ row entity to the column entity and the matched indices are not duplicated
in each row of ColToRowMatchIndices. If the column entity is not matched
any row entity, set -1 in ColToRowMatchIndices.

Please note that the input DistMat can be phi::DenseTensor (with LoD) or Tensor.
Please note that the input DistMat can be phi::DenseTensor (with LoD) or phi::DenseTensor.
Copy link
Contributor

Choose a reason for hiding this comment

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

phi::DenseTensor (with LoD) or phi::DenseTensor 合并为一处DenseTensor即可

Copy link
Contributor Author

@Liyulingyue Liyulingyue Nov 30, 2022

Choose a reason for hiding this comment

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

请确认第307-308行的内容,此处的or应该不能轻易删除。 @zyfncg

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, 不用合并

Comment on lines 34 to 49
class AdamaxOpMaker : public framework::OpProtoAndCheckerMaker {
public:
void Make() override {
AddInput("Param", "(Tensor) Input parameter");
AddInput("Grad", "(Tensor) Input gradient");
AddInput("LearningRate", "(Tensor) Learning rate");
AddInput("Moment", "(Tensor) First moment");
AddInput("Param", "(phi::DenseTensor) Input parameter");
AddInput("Grad", "(phi::DenseTensor) Input gradient");
AddInput("LearningRate", "(phi::DenseTensor) Learning rate");
AddInput("Moment", "(phi::DenseTensor) First moment");
AddInput("InfNorm",
"(Tensor) "
"(phi::DenseTensor) "
"Input exponentially weighted infinity norm");
AddInput("Beta1Pow", "(Tensor) Input beta1 power accumulator");
AddInput("Beta1Pow", "(phi::DenseTensor) Input beta1 power accumulator");

AddOutput("ParamOut", "(Tensor) Output parameter");
AddOutput("MomentOut", "(Tensor) Output first moment");
AddOutput("ParamOut", "(phi::DenseTensor) Output parameter");
AddOutput("MomentOut", "(phi::DenseTensor) Output first moment");
AddOutput("InfNormOut",
"(Tensor) "
"(phi::DenseTensor) "
Copy link
Contributor

Choose a reason for hiding this comment

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

由于OpMaker中的部分参数不仅支持DenseTensor类型,也支持SelectedRows类型,因此目前OpMaker中注释的Tensor写法建议保留,暂不替换为DenseTensor

Copy link
Contributor

@zyfncg zyfncg Nov 30, 2022

Choose a reason for hiding this comment

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

其他文件(xxx_op.cc)中的OpMaker相关的代码也建议暂时先不做替换,有哪些算子的哪些参数支持SelectedRows目前不太好一一排查,修改之后可能会出现误导的情况

Copy link
Contributor Author

Choose a reason for hiding this comment

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

其他文件(xxx_op.cc)中的OpMaker相关的代码也建议暂时先不做替换,有哪些算子的哪些参数支持SelectedRows目前不太好一一排查,修改之后可能会出现误导的情况

鉴于此种情况,我建议关闭这个pr,然后通过新的规则进行新的pr。请确认以下,除了(xxx_op.cc)中的OpMaker相关的代码外,还有.h和.cu中也有类似情况吗?

Copy link
Contributor

@zyfncg zyfncg Nov 30, 2022

Choose a reason for hiding this comment

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

.cu和.h可以不用考虑
.cu中不会出现OpMaker的代码;.h文件中可能会有,不过看了下这个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.

.cu和.h可以不用考虑 .cu中不会出现OpMaker的代码;.h文件中可能会有,不过看了下这个PR里没有涉及到

对,这个pr主要在尝试去除using Tensor = phi::DenseTensor的使用,计划合入后去除.h和.cu以及剩余通过include方式引入Tensor的.cc文件中的Tensor。现在考虑到需要避开OpMaker,因此计划新开PR进行一次性剔除。

那么.h文件中需要对Opmaker进行规避吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

.h也需要,在Opmaker内的目前都先不做改动吧

@paddle-bot
Copy link

paddle-bot bot commented Dec 3, 2022

很抱歉,经过我们的反复讨论,你的PR暂未达到合入标准,请阅读飞桨原生算子开发规范,你可以重新提交新的PR,我们先将此PR关闭,感谢你的贡献。
Sorry to inform you that through our discussion, your PR fails to meet the merging standard (Reference: Paddle Custom Operator Design Doc). You can also submit an new one. Thank you.

@Liyulingyue Liyulingyue deleted the NewDenseTensor_for_cc branch January 4, 2023 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants