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

【Hackathon 4 No.19】Add polygamma API to Paddle #53791

Merged
merged 15 commits into from
Jun 5, 2023

Conversation

PommesPeter
Copy link
Contributor

@PommesPeter PommesPeter commented May 14, 2023

PR types

New features

PR changes

APIs

Description

rfc doc here: PaddlePaddle/community#472
updated rfc doc here: PaddlePaddle/community#542
polygamma doc here: PaddlePaddle/docs#5913

@paddle-bot
Copy link

paddle-bot bot commented May 14, 2023

你的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 May 14, 2023
@paddle-bot
Copy link

paddle-bot bot commented May 14, 2023

❌ The PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

@PommesPeter
Copy link
Contributor Author

PommesPeter commented May 22, 2023

https://github.com/PaddlePaddle/Paddle/blob/91a77fe079ad334e75a122168a8338fa3acd0a97/python/paddle/tensor/math.py#L5684-L5686
我想反馈一个 bug,出自于 paddle.digamma 的,因为本 api 中, n=0 的情况是可以直接调用 digamma api,但是 digammax=0 处的返回值和 scipy 的 special.digamma 并没有对齐,torch 也同理。

测试的结果如下:

  • scipy
    image

  • torch
    image

  • paddle
    image

这是否可认为是一个 bug,根据定义 x=0 处的值应为 -inf,但 paddle 返回了 nan@luotao1 @zoooo0820 @Ligoml

@zoooo0820
Copy link
Contributor

这是否可认为是一个 bug,根据定义 x=0 处的值应为 -inf,但 paddle 返回了 nan

@PommesPeter 你好,这里的确是有行为上的差异,torch从1.8版本开始将x=0点的输出从nan改为了-inf。想确认下这个行为差异会对polygamma的实现带来多少影响?除了x=0处输出的结果外,还会有其他的差异吗。

@zoooo0820
Copy link
Contributor

@PommesPeter 另外,从目前的实现看,新增了相关OP,这与RFC方案中基于现有API组合实现的方式有差异。出于飞桨多硬件适配等方面的考虑,是期望尽量减少基础算子的数量的。请问这里新增算子是否是因为遇到了RFC方案不可解决的问题,或是有其他特殊的考虑吗?

@PommesPeter
Copy link
Contributor Author

这是否可认为是一个 bug,根据定义 x=0 处的值应为 -inf,但 paddle 返回了 nan

@PommesPeter 你好,这里的确是有行为上的差异,torch从1.8版本开始将x=0点的输出从nan改为了-inf。想确认下这个行为差异会对polygamma的实现带来多少影响?除了x=0处输出的结果外,还会有其他的差异吗。

目前只发现 x=0 处的结果有问题,其他的不会有差异。

@PommesPeter
Copy link
Contributor Author

@PommesPeter 另外,从目前的实现看,新增了相关OP,这与RFC方案中基于现有API组合实现的方式有差异。出于飞桨多硬件适配等方面的考虑,是期望尽量减少基础算子的数量的。请问这里新增算子是否是因为遇到了RFC方案不可解决的问题,或是有其他特殊的考虑吗?

是看到该算子初期的任务要求是需要使用 C++ 实现,参考 torch 实现后,发现使用 zeta 函数能够减少递归带来的运算量,rfc 文档里面预期是采用递归的方式,但该操作性能影响可能会较大,考虑算子计算性能故参考 torch 的实现。

@zoooo0820
Copy link
Contributor

是看到该算子初期的任务要求是需要使用 C++ 实现,参考 torch 实现后,发现使用 zeta 函数能够减少递归带来的运算量,rfc 文档里面预期是采用递归的方式,但该操作性能影响可能会较大,考虑算子计算性能故参考 torch 的实现。

辛苦也对应在RFC文档中,提交PR修改下OP正反向相关计算逻辑的介绍吧

@PommesPeter
Copy link
Contributor Author

PommesPeter commented May 23, 2023

是看到该算子初期的任务要求是需要使用 C++ 实现,参考 torch 实现后,发现使用 zeta 函数能够减少递归带来的运算量,rfc 文档里面预期是采用递归的方式,但该操作性能影响可能会较大,考虑算子计算性能故参考 torch 的实现。

辛苦也对应在RFC文档中,提交PR修改下OP正反向相关计算逻辑的介绍吧

好的,已更新 PaddlePaddle/community#542

@PommesPeter
Copy link
Contributor Author

rfc 文档已更新,麻烦 review 一下 @zoooo0820

@zoooo0820
Copy link
Contributor

目前只发现 x=0 处的结果有问题,其他的不会有差异。

@PommesPeter 你好,关于paddle.digamma在0点的行为问题,初步评估了一下,个人认为这里不需要专门改动digamma的行为,主要的理由是:

  1. 从数学的角度,0及负整数点,都属于第二类间断点,本身未定义函数值,且间断点两边极限不存在。所以个人认为,NaN是符合相关数学意义的,不能简单定义为Bug。
  2. scipy/pytorch在1所述的场景上,本身行为也有相关的演化过程。目前区分了0和-0,分别返回-inf / inf,这表示对应的极限值。根据其演化过程中产生的讨论来看,这个改动本身也是存在一定的争议,因为看起来都是合理的。
  3. 从影响面看,仅一个特殊值点存在一些差异,且这个值实际不应是一个合法输入。

@PommesPeter
Copy link
Contributor Author

目前只发现 x=0 处的结果有问题,其他的不会有差异。

@PommesPeter 你好,关于paddle.digamma在0点的行为问题,初步评估了一下,个人认为这里不需要专门改动digamma的行为,主要的理由是:

  1. 从数学的角度,0及负整数点,都属于第二类间断点,本身未定义函数值,且间断点两边极限不存在。所以个人认为,NaN是符合相关数学意义的,不能简单定义为Bug。
  2. scipy/pytorch在1所述的场景上,本身行为也有相关的演化过程。目前区分了0和-0,分别返回-inf / inf,这表示对应的极限值。根据其演化过程中产生的讨论来看,这个改动本身也是存在一定的争议,因为看起来都是合理的。
  3. 从影响面看,仅一个特殊值点存在一些差异,且这个值实际不应是一个合法输入。

那目前的解决方案是继续保持该点的取值为 nan,剔除 zero case 的情况么?因为这个实现和目前 scipy 和 pytorch 不统一。目前 CI 仅存在该点的误差。

通过调研情况来看,x=0 是一个没有定义值的,那是否应该对 x=0 的情况做异常值处理,不允许 x=0 作为输入。

@zoooo0820
Copy link
Contributor

目前只发现 x=0 处的结果有问题,其他的不会有差异。

@PommesPeter 你好,关于paddle.digamma在0点的行为问题,初步评估了一下,个人认为这里不需要专门改动digamma的行为,主要的理由是:

  1. 从数学的角度,0及负整数点,都属于第二类间断点,本身未定义函数值,且间断点两边极限不存在。所以个人认为,NaN是符合相关数学意义的,不能简单定义为Bug。
  2. scipy/pytorch在1所述的场景上,本身行为也有相关的演化过程。目前区分了0和-0,分别返回-inf / inf,这表示对应的极限值。根据其演化过程中产生的讨论来看,这个改动本身也是存在一定的争议,因为看起来都是合理的。
  3. 从影响面看,仅一个特殊值点存在一些差异,且这个值实际不应是一个合法输入。

那目前的解决方案是继续保持该点的取值为 nan,剔除 zero case 的情况么?因为这个实现和目前 scipy 和 pytorch 不统一。目前 CI 仅存在该点的误差。

通过调研情况来看,x=0 是一个没有定义值的,那是否应该对 x=0 的情况做异常值处理,不允许 x=0 作为输入。

当前保持现状即可,nan本身也有异常值的意义,n=0时对所有x直接复用digamma是可行的吧。

@PommesPeter
Copy link
Contributor Author

目前只发现 x=0 处的结果有问题,其他的不会有差异。

@PommesPeter 你好,关于paddle.digamma在0点的行为问题,初步评估了一下,个人认为这里不需要专门改动digamma的行为,主要的理由是:

  1. 从数学的角度,0及负整数点,都属于第二类间断点,本身未定义函数值,且间断点两边极限不存在。所以个人认为,NaN是符合相关数学意义的,不能简单定义为Bug。
  2. scipy/pytorch在1所述的场景上,本身行为也有相关的演化过程。目前区分了0和-0,分别返回-inf / inf,这表示对应的极限值。根据其演化过程中产生的讨论来看,这个改动本身也是存在一定的争议,因为看起来都是合理的。
  3. 从影响面看,仅一个特殊值点存在一些差异,且这个值实际不应是一个合法输入。

那目前的解决方案是继续保持该点的取值为 nan,剔除 zero case 的情况么?因为这个实现和目前 scipy 和 pytorch 不统一。目前 CI 仅存在该点的误差。
通过调研情况来看,x=0 是一个没有定义值的,那是否应该对 x=0 的情况做异常值处理,不允许 x=0 作为输入。

当前保持现状即可,nan本身也有异常值的意义,n=0时对所有x直接复用digamma是可行的吧。

对的,是可以直接复用 digamma,那我参考 digamma 的单测让 CI 通过。

@zoooo0820
Copy link
Contributor

那我参考 digamma 的单测让 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-Approval 有一条关于检测到使用了std::cout / print的报告,辛苦再确认下是否是误报呢。


} // namespace phi

PD_REGISTER_KERNEL(
Copy link
Contributor

Choose a reason for hiding this comment

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

这里是否可以扩展更多dtype呢,如fp16等。kernel注册dtype时,尽量把目前理论上应当支持的,同时Paddle框架机制上也支持的dtype都包含进来,否则在某些特定场景会有问题。

在RFC设计阶段中,只支持fp32/fp64的原因主要是此前的方案是在digamma上进行,而digamma kernel支持的有限。这个后续也会通过其他专项任务去逐步扩展

return _C_ops.polygamma(x, n)
else:
check_variable_and_dtype(
x, "x", ["float32", "float64"], "polygamma"
Copy link
Contributor

Choose a reason for hiding this comment

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

kernel数据类型扩展后,这里可以相应放宽数据类型的支持

python/paddle/fluid/tests/unittests/test_polygamma_op.py Outdated Show resolved Hide resolved
python/paddle/fluid/tests/unittests/test_polygamma_op.py Outdated Show resolved Hide resolved
@luotao1
Copy link
Contributor

luotao1 commented May 31, 2023

CI-Approval 有一条关于检测到使用了std::cout / print的报告,辛苦再确认下是否是误报呢。

是代码示例中用了print, @tianshuo78520a 在做规则增强,等该 PR 全部 ready 后,可以豁免

@PommesPeter
Copy link
Contributor Author

CI-Approval 有一条关于检测到使用了std::cout / print的报告,辛苦再确认下是否是误报呢。

是代码示例中用了print, @tianshuo78520a 在做规则增强,等该 PR 全部 ready 后,可以豁免

好的

zoooo0820
zoooo0820 previously approved these changes Jun 2, 2023
@luotao1
Copy link
Contributor

luotao1 commented Jun 2, 2023

请修复下CI问题,可以merge develop

@PommesPeter
Copy link
Contributor Author

请修复下CI问题,可以merge develop

好的,正在修复

@luotao1
Copy link
Contributor

luotao1 commented Jun 3, 2023

image 单测覆盖率不够,请补充单测

@PommesPeter
Copy link
Contributor Author

image 单测覆盖率不够,请补充单测

好的

zoooo0820
zoooo0820 previously approved these changes Jun 5, 2023
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

PommesPeter and others added 2 commits June 5, 2023 12:20
Co-authored-by: zachary sun <70642955+sunzhongkai588@users.noreply.github.com>
Copy link
Contributor

@sunzhongkai588 sunzhongkai588 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

Copy link
Contributor

@sunzhongkai588 sunzhongkai588 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

Copy link
Contributor

@jeff41404 jeff41404 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

This comment was marked as duplicate.

@luotao1 luotao1 merged commit ed60456 into PaddlePaddle:develop Jun 5, 2023
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants