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

【Hackathon 4 No.20】Add i0 / i0e to paddle #52058

Merged
merged 35 commits into from
May 12, 2023
Merged

【Hackathon 4 No.20】Add i0 / i0e to paddle #52058

merged 35 commits into from
May 12, 2023

Conversation

PommesPeter
Copy link
Contributor

@PommesPeter PommesPeter commented Mar 23, 2023 •
edited
Loading

PR types

New features

PR changes

APIs

Description

added i0 / i0e API to paddle

RFC doc here: PaddlePaddle/community#471
updated RFC doc here: PaddlePaddle/community#488
[4/26/2023] update RFC doc here: PaddlePaddle/community#525
Chinese docs here: PaddlePaddle/docs#5838

@paddle-bot
Copy link

paddle-bot bot commented Mar 23, 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 Mar 23, 2023
@paddle-bot
Copy link

paddle-bot bot commented Mar 23, 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 PommesPeter changed the title I0 【Hackathon 4 No.20】Add i0 / i0e to paddle Mar 23, 2023
@zhengqiwen1997
Copy link
Contributor

请修改，保证Required的CI通过。

@PommesPeter
Copy link
Contributor Author

请修改，保证Required的CI通过。

好的 正在修复问题

@zhengqiwen1997
Copy link
Contributor

项目进行的怎么样了？

@PommesPeter
Copy link
Contributor Author

PommesPeter commented Apr 7, 2023 •
edited
Loading

项目进行的怎么样了？

目前仍有一些问题，已经引入 Eigen3 库的实现，但是在编译时报错，原因为没有找到相应的 reference。应该是模板用法不对，目前仍在排查。

fd261a50139261bae49a12a622d264a

@PommesPeter
Copy link
Contributor Author

@zhengqiwen1997 现在动态图的算子已经可以跑通了，而且结果正确，但静态图遇到了这个报错，请问是什么原因造成呢？我也重新 cmake 了，还是不行。这个报错看起来是没有认到新增的这个算子，是不是漏改了什么地方么？
image

@PommesPeter
Copy link
Contributor Author

@zhengqiwen1997 现在动态图的算子已经可以跑通了，而且结果正确，但静态图遇到了这个报错，请问是什么原因造成呢？我也重新 cmake 了，还是不行。这个报错看起来是没有认到新增的这个算子，是不是漏改了什么地方么？ image

该问题已解决

luotao1
luotao1 reviewed Apr 21, 2023
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.

需要添加C++ 算子单元测试 来通过覆盖率检查

@PommesPeter
Copy link
Contributor Author

需要添加C++ 算子单元测试 来通过覆盖率检查

好的

@PommesPeter PommesPeter mentioned this pull request Apr 21, 2023
Closed
@PommesPeter
Copy link
Contributor Author

覆盖率测试已过，麻烦 review 一下 @zhengqiwen1997 @luotao1

@zhengqiwen1997
Copy link
Contributor

PR-CI-OP-benchmark 为什么没通过呢

@luotao1
Copy link
Contributor

luotao1 commented Apr 25, 2023

PR-CI-OP-benchmark 为什么没通过呢

应该是触发了跑全量Op，然后超时了，不影响review

zhengqiwen1997
zhengqiwen1997 previously approved these changes Apr 25, 2023
View reviewed changes
@PommesPeter
Copy link
Contributor Author

PommesPeter commented Apr 25, 2023 •
edited
Loading

PR-CI-OP-benchmark 为什么没通过呢

应该是触发了跑全量Op，然后超时了，不影响review

好的，那我补充一下该 OP 文档。

@luotao1 luotao1 added the API label Apr 25, 2023
ZzSean
ZzSean previously approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@ZzSean ZzSean 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 CI-OP-Benchmark

@PommesPeter
Copy link
Contributor Author

PommesPeter commented Apr 25, 2023 •
edited
Loading

中文文档已提 pr PaddlePaddle/docs#5838

@zhengqiwen1997
Copy link
Contributor

经过我们内部讨论，使用Eigen可能不支持后续高维度Tensor的开展。我们目前是写死的最多9维，而未来我们需要扩展支持高维度Tensor。而Eigen不支持动态rank，EigenTensor在创建时需要指定维度，后续扩展困难。所以能麻烦你修改成不依赖任何Eigen的版本吗？应该需要用你原来的版本了，设计文档也应先修改。
给你的开发带来的不便十分抱歉！但请放心技术approve了，其他人不能做这道题。所以请花些时间进行修改。

@PommesPeter
Copy link
Contributor Author

经过我们内部讨论，使用Eigen可能不支持后续高维度Tensor的开展。我们目前是写死的最多9维，而未来我们需要扩展支持高维度Tensor。而Eigen不支持动态rank，EigenTensor在创建时需要指定维度，后续扩展困难。所以能麻烦你修改成不依赖任何Eigen的版本吗？应该需要用你原来的版本了，设计文档也应先修改。 给你的开发带来的不便十分抱歉！但请放心技术approve了，其他人不能做这道题。所以请花些时间进行修改。

好的，那我先修改设计文档，设计文档回到之前的版本是否可行，然后在之前版本补充反向计算过程（之前对于反向的描述比较简略）

PommesPeter and others added 17 commits May 10, 2023 13:59
@PommesPeter
refactor: updated code style
d42468b
@PommesPeter
refactor: updated op_compat.yaml
22f1004
@PommesPeter
refactor: updated op_compat.yaml
66b5fd1
@PommesPeter
refactor: fixed op name mapping and optimize unittest case
924b3fd
@PommesPeter
refactor: manually implement i0 / i0e
08ecc31
@PommesPeter
refactor: added grad kernel for i0 / i0e,didn't finish
cff862c
@PommesPeter
Update math.py
6c57f75
@PommesPeter
refactor: added equation to doc in English and added comments for com…
18757b7 
…puting i0 / i0e gradient
@PommesPeter
refactor: removed eigen implementation
db95300
@PommesPeter
refactor: finished i0 / i0e cpu and gpu op
74a1404
@PommesPeter
refactor: updated code style
2082bfb
@PommesPeter
fix: find a bug but not fix
75a7d69
@PommesPeter
fix: incorrect unittest cases
4aee598
@PommesPeter
update: updated code style and remove my file
e665d31
@PommesPeter
update: updated unittest case
90f0672
@PommesPeter
fix: fixed sign error
eeca07d
@PommesPeter
fix: fixed mistakes when merging
d3fb6f1
@PommesPeter
Copy link
Contributor Author

@zhengqiwen1997 CI 已通过，麻烦 review 一下，辛苦了！

@PommesPeter
Copy link
Contributor Author

@luotao1 i0 和 i1 都技术 approve 了，麻烦进行下一个阶段审核

sunzhongkai588
sunzhongkai588 approved these changes May 12, 2023
View reviewed changes
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

jeff41404
jeff41404 approved these changes May 12, 2023
View reviewed changes
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

@luotao1 luotao1 merged commit ce256f7 into PaddlePaddle:develop May 12, 2023
@xuxinyi389 xuxinyi389 mentioned this pull request Dec 20, 2023
Merged
@jeff41404 jeff41404 mentioned this pull request Jan 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luotao1 luotao1 luotao1 left review comments

@LyndonKong LyndonKong LyndonKong left review comments

@jeff41404 jeff41404 jeff41404 approved these changes

@sunzhongkai588 sunzhongkai588 sunzhongkai588 approved these changes

@zhengqiwen1997 zhengqiwen1997 zhengqiwen1997 approved these changes

@ZzSean ZzSean ZzSean left review comments

Assignees

@luotao1 luotao1

@jeff41404 jeff41404

@Ligoml Ligoml

@zhengqiwen1997 zhengqiwen1997

Labels
API contributor External developers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@PommesPeter @zhengqiwen1997 @luotao1 @jeff41404 @ZzSean @LyndonKong @sunzhongkai588 @Ligoml