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 4th No.12】为 Paddle 新增 Cauchy API #52999

Merged
merged 7 commits into from
Apr 28, 2023

Conversation

megemini
Copy link
Contributor

@megemini megemini commented Apr 17, 2023

PR types

New features

PR changes

APIs

Description

[used AI Studio]

  • 飞桨黑客马拉松第四期 No.12:为 Paddle 新增 Cauchy API
  • 涉及以下5个文件:
    • __init__.py: 将 Cauchy 添加到 distribution 目录下
    • cauchy.py: Cauchy API 的具体实现
    • kl.py: 添加 Cauchy 对应的 kl_divergence 方法
    • test_distribution_cauchy.py: 动态图单元测试,含kl.py
    • test_distribution_cauchy_static.py: 静态图单元测试,含kl.py

其他说明:

  • 以上代码通过 pre-commit
  • 以上代码已经在 AI Studio 测试通过
  • 自测代码覆盖率 90%+

@luotao1 @cxxly @Ligoml @dasenCoding

谢谢评审!

中文文档:PaddlePaddle/docs#5807

RFC:@dasenCoding

p.s. 2023-4-25 Update

  • 修改测试用例目录。
  • 增加部分动态图测试用例,目前coverage 100%。
  • 修改部分测试用例,适配 to_tensor 默认 0D 而不是 1D 的行为。

请评审,谢谢!:)

p.s. 2023-4-21 Update

  • 参考review重写了 __init__ 方法。
  • 不使用 check_type ,改用 isinstance 做类型检查。
  • 去掉取值范围的检查。
  • 对应修改相应测试用例。

请评审,谢谢!:)

@paddle-bot
Copy link

paddle-bot bot commented Apr 17, 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.

from paddle.fluid.data_feeder import check_type, convert_dtype
from paddle.fluid.framework import _non_static_mode
from paddle.fluid.layers import tensor

Copy link
Contributor

Choose a reason for hiding this comment

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

导入规范遵循pep8,不意见直接导入函数、类

'scale',
(float, tensor.Variable),
'Cauchy',
)
Copy link
Contributor

@cxxly cxxly Apr 20, 2023

Choose a reason for hiding this comment

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

直接使用 isinstance(loc, (numbers.Real, framework.Variable)) 进行类型检查没,不需要if not _non_static_mode()判断

self.scale = scale
self.dtype = convert_dtype(loc.dtype)
else:
[self.loc, self.scale] = self._to_tensor(loc, scale)
Copy link
Contributor

Choose a reason for hiding this comment

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

distribution._validate_args, distribution._to_tensor是遗留API,存在缺陷,建议参考 gumbel.py 写法

if _non_static_mode():
"""Not use `paddle.any` in static mode, which always be `True`."""
if paddle.any(self.scale <= 0):
raise ValueError("The arg of `scale` must be positive.")
Copy link
Contributor

Choose a reason for hiding this comment

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

为了避免性能问题,建议移除此判断;后续 distribution全面支持参数校验,再进行此判断

raise ValueError("The arg of `scale` must be positive.")

super().__init__(
batch_shape=(self.loc + self.scale).shape, event_shape=()
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.

利用broadcast获取到batch shape,有问题吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我根据 gumbel.py 把初始化重新写一下吧~ :)

Copy link
Contributor

Choose a reason for hiding this comment

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

可以用 broadcast_shape,加法会有开销

@megemini
Copy link
Contributor Author

刚看到 PR-CI-Coverage 出错了,不太清楚是什么问题,我在本地的coverage在90%+

Name                          Stmts   Miss  Cover
-------------------------------------------------
cauchy.py                        74      6    92%
test_distribution_cauchy.py     159      2    99%

Name                                 Stmts   Miss  Cover
--------------------------------------------------------
cauchy.py                               74      0   100%
test_distribution_cauchy_static.py     165      0   100%

请帮忙看看,谢谢!:)

@cxxly
Copy link
Contributor

cxxly commented Apr 24, 2023

刚看到 PR-CI-Coverage 出错了,不太清楚是什么问题,我在本地的coverage在90%+

Name                          Stmts   Miss  Cover
-------------------------------------------------
cauchy.py                        74      6    92%
test_distribution_cauchy.py     159      2    99%

Name                                 Stmts   Miss  Cover
--------------------------------------------------------
cauchy.py                               74      0   100%
test_distribution_cauchy_static.py     165      0   100%

请帮忙看看,谢谢!:)

image
主要是一些类型检查代码没覆盖到,点击CI能够看到

@megemini
Copy link
Contributor Author

image 主要是一些类型检查代码没覆盖到,点击CI能够看到

这里面 Line data1 的表示覆盖到, 0 表示没覆盖到?

我怎么看里面除了方法名之外都是 0 ... ...

我重新 commit 一下吧~

@megemini
Copy link
Contributor Author

@cxxly 刚才看了一下,好象是所有的测试用例目录变了,导致我的测试用例应该是没加进去。

原来是:home/aistudio/Paddle/python/paddle/fluid/tests/unittests/distribution
现在是:home/aistudio/Paddle/test/distribution

我把我的用例移过去重新commit吧~

@megemini
Copy link
Contributor Author

CI 基本都通过了,就是这个 CheckPRTemplate 不知道为啥失败了~

请评审,谢谢!:)

Copy link
Contributor

@cxxly cxxly left a comment

Choose a reason for hiding this comment

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

LGTM

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

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

@luotao1 luotao1 merged commit 896c931 into PaddlePaddle:develop Apr 28, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants