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

【PaddlePaddle Hackathon 2】18、为 Paddle 新增 paddle.heaviside 和 paddle.Tensor.heaviside API #41872

Merged
merged 19 commits into from
May 10, 2022
Merged

【PaddlePaddle Hackathon 2】18、为 Paddle 新增 paddle.heaviside 和 paddle.Tensor.heaviside API #41872

merged 19 commits into from
May 10, 2022

Conversation

BrilliantYuKaimin
Copy link
Contributor

PR types

New features

PR changes

APIs

Describe

解决了 issue：#40315
实现了 paddle.heaviside。paddle.heaviside(x, y) 在 x>0 时返回 1，在 x=0 时返回 y，在 x<0 时返回 0。
设计文档：PaddlePaddle/community#57
中文文档：PaddlePaddle/docs#4641

@paddle-bot-old paddle-bot-old bot added contributor External developers status: proposed labels Apr 16, 2022
@paddle-bot-old
Copy link

你的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.

This was referenced Apr 16, 2022
Closed
Merged
@zoooo0820 zoooo0820 self-assigned this Apr 18, 2022
zoooo0820
zoooo0820 reviewed Apr 18, 2022
View reviewed changes
Copy link
Contributor

@zoooo0820 zoooo0820 left a comment

Choose a reason for hiding this comment

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

关于该API的PR，前序讨论及修改记录请见 #40934

  • 当前还有一个小地方的问题，修改完成即可

python/paddle/__init__.py
@@ -268,6 +268,7 @@
from .tensor.math import fmin # noqa: F401
from .tensor.math import inner # noqa: F401
from .tensor.math import outer # noqa: F401
from .tensor.math import heaviside # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

这里请也将heaviside添加到这个文件的__all__列表中，使得paddle.heviside作为公开API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

@BrilliantYuKaimin
Copy link
Contributor Author

现在PR-CI-Coverage显示C++代码的行覆盖率为41/46=89.1%，不足90%。

我们可以看到paddle/fluid/operators/elementwise/elementwise_heaviside_op.cc中最后一行的REGISTER_OPERATOR(elementwise_heaviside_grad, ops::ElementwiseOpGrad);是没有被命中的，而且paddle/phi/kernels/cpu/elementwise_grad_kernel.cc也没有出现在这里，这是不是说明梯度算子没有被测试到？但是单测python/paddle/fluid/tests/unittests/test_elementwise_heaviside_op.py里是有check_grad的，难道这里没有调用到梯度算子吗？

希望评审老师能解答一下或者再给点修改建议。

@zoooo0820
Copy link
Contributor

现在PR-CI-Coverage显示C++代码的行覆盖率为41/46=89.1%，不足90%。

我们可以看到paddle/fluid/operators/elementwise/elementwise_heaviside_op.cc中最后一行的REGISTER_OPERATOR(elementwise_heaviside_grad, ops::ElementwiseOpGrad);是没有被命中的，而且paddle/phi/kernels/cpu/elementwise_grad_kernel.cc也没有出现在这里，这是不是说明梯度算子没有被测试到？但是单测python/paddle/fluid/tests/unittests/test_elementwise_heaviside_op.py里是有check_grad的，难道这里没有调用到梯度算子吗？

希望评审老师能解答一下或者再给点修改建议。

这个ci是elementwise_sig.cc的问题，已找单测相关同学处理完成

zoooo0820
zoooo0820 previously approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@zoooo0820 zoooo0820 left a comment

Choose a reason for hiding this comment

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

LGTM

@zoooo0820
Copy link
Contributor

请再处理一下Docs文档生成的ci问题，生成正确的文档链接后继续下面的review流程

@paddle-bot-old
Copy link

PR格式检查通过，你的PR将接受Paddle专家以及开源社区的review，请及时关注PR动态。
The format inspection passed. Your PR will be reviewed by experts of Paddle and developers from the open-source community. Stay tuned.

@BrilliantYuKaimin
Copy link
Contributor Author

请再处理一下Docs文档生成的ci问题，生成正确的文档链接后继续下面的review流程

已经生成正确的文档。

DDDivano
DDDivano previously approved these changes Apr 27, 2022
View reviewed changes
@dingjiaweiww dingjiaweiww mentioned this pull request Apr 28, 2022
Closed
TCChenlong
TCChenlong previously approved these changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@TCChenlong TCChenlong left a comment

Choose a reason for hiding this comment

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

LGTM for api docs

zoooo0820
zoooo0820 previously approved these changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@zoooo0820 zoooo0820 left a comment

Choose a reason for hiding this comment

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

LGTM

luotao1
luotao1 previously approved these changes Apr 28, 2022
View reviewed changes
@paddle-bot-old
Copy link

paddle-bot-old bot commented May 1, 2022

Sorry to inform you that 12ebfdb's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

Ligoml
Ligoml approved these changes May 6, 2022
View reviewed changes
Copy link
Contributor

@Ligoml Ligoml left a comment

Choose a reason for hiding this comment

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

LGTM for docs

luotao1
luotao1 approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

LGTM

XiaoguangHu01
XiaoguangHu01 approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

@jeff41404 jeff41404 merged commit 4892d59 into PaddlePaddle:develop May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XiaoguangHu01 XiaoguangHu01 XiaoguangHu01 approved these changes

@Ligoml Ligoml Ligoml approved these changes

@zoooo0820 zoooo0820 zoooo0820 approved these changes

@jeff41404 jeff41404 jeff41404 approved these changes

@luotao1 luotao1 luotao1 approved these changes

@TCChenlong TCChenlong TCChenlong left review comments

@DDDivano DDDivano DDDivano left review comments

Assignees

@zoooo0820 zoooo0820

Labels
contributor External developers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@BrilliantYuKaimin @zoooo0820 @luotao1 @jeff41404 @DDDivano @TCChenlong @Ligoml @XiaoguangHu01 @dingjiaweiww