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

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

Closed

Conversation

BrilliantYuKaimin
Copy link
Contributor

@BrilliantYuKaimin BrilliantYuKaimin commented Mar 25, 2022

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

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.

目前尚有几个CI未能通过,需要check下CI未过的原因,整体看主要是单测超时的问题需要解决。

@@ -0,0 +1,112 @@
# Copyright (c) 2022 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

单测中需要对照设计文档中补充测试case,目前仍然缺少不同dtype/设备/动态图、错误检查等测试内容。
此外,因为check_grad耗时比较多,可以适当减少OPTEST的用例。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已经补充了测试内容,OPTEST减少到了3个。

}

void AddInputY() override {
AddInput("Y", "The tensor determining a Heaviside step function.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

@dingjiaweiww
Copy link
Contributor

请先通过CI噢~

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.

目前CI仍然有几个问题:

  1. static-check: 代码格式问题,请参考CI提示的位置修改代码,或是再用pre-commit刷一下格式
  2. coverage:单测仍然提示超时,可以尝试在保证单测覆盖率的情况下再适当减少一些OPTest的使用
  3. Approval:请再确认一下对单测精度阈值的修改是否必要,如是则需要相关负责人的approve.

op_type = 'elementwise_heaviside'
axis = -1
act = None
if paddle.in_dynamic_mode():
Copy link
Contributor

Choose a reason for hiding this comment

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

这里由于最近的paddle内部的一些改动,辛苦将paddle.in_dynamic_mode()替换为_non_static_mode, 该方法在https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/tensor/math.py#L32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成


def test_check_grad_ingore_y(self):
self.check_grad(
['X'], 'Out', max_relative_error=0.005, no_grad_set=set('Y'))
Copy link
Contributor

Choose a reason for hiding this comment

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

这里如果使用默认设置max_relative_error会出现单测过不了的情况吗,修改默认对比精度需要专门的负责人Approve以通过CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

max_relative_error的默认值也是0.005,我已经在这里把这个参数删去。

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.

目前仍然需要处理下coverage和static两个ci的问题。
目前代码和线上分支出现了冲突,先merge下最新的develop分支解下冲突;另外static ci显示仍然存在一些代码风格问题。

@BrilliantYuKaimin
Copy link
Contributor Author

目前仍然需要处理下coverage和static两个ci的问题。
目前代码和线上分支出现了冲突,先merge下最新的develop分支解下冲突;另外static ci显示仍然存在一些代码风格问题。

已经处理完成。

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.

CI问题已经解决,还有个别API使用规范问题需要解决一下

class TestHeavisideError(unittest.TestCase):
def test_input(self):
def test_input_x():
with paddle.fluid.dygraph.guard():
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 动态图下这个guard可以去掉,下同
  • 如果报静态图的错可以先使用paddle.disable_static切换到动态图

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

from op_test import OpTest
import paddle
import paddle.fluid.core as core
import paddle.fluid as fluid
Copy link
Contributor

Choose a reason for hiding this comment

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

目前fluid是目前正在逐步执行废弃计划的API,已有的也在逐步清理中。这里辛苦将这个单测文件中使用fluid相关的API统一替换为Paddle公开的API,避免增加后续清理工作。

  • 静态图运行API请参考paddle.static目录下的API
  • 设备检查API可以使用paddle.device.is_compiled_with_cuda()

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

BrilliantYuKaimin commented Apr 14, 2022

现在既有paddle/phi/kernels/elementwise_kernel.cc又有paddle/phi/kernels/cpu/elementwise_kernel.cc,而paddle/phi/kernels/gpu/elementwise_kernel.cu已被删除,该如何安排elementwise_heaviside的位置?

@BrilliantYuKaimin
Copy link
Contributor Author

由于出现了不好解决的复杂冲突,我重新提了一个 PR:#41872

@paddle-bot-old
Copy link

很抱歉,经过我们的反复讨论,你的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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants