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.20&No.21】Add paddle.i1 / paddle.i1e API rfc doc, update paddle.i0 / paddle.i0e API rfc doc. #478

Closed

Conversation

LyndonKong
Copy link
Contributor

@LyndonKong LyndonKong commented Mar 24, 2023

Changes:

  • update i0 / i0e api rfc doc.
  • add i1 / i1e api rfc doc.

Contribution:

  • This version of rfc document also consider the implementation of Tensorflow and choose eigen library as the solution which is more convenient.
  • We also design the backward grad op for these 4 operators.

Consider the differences in backward grad design, we split i0 and i0e into different API rfc doc which is different to the previous version
Since the implementation of these two tasks are highly relevant in implementation, we decide to put them into one single pr.

@paddle-bot
Copy link

paddle-bot bot commented Mar 24, 2023

你的PR提交成功,感谢你对开源项目的贡献!
请检查PR提交格式和内容是否完备,具体请参考示例模版
Your PR has been submitted. Thanks for your contribution!
Please check its format and content. For this, you can refer to Template and Demo.

@LyndonKong LyndonKong changed the title 【Hackathon 4th No.20&No.21】Added paddle.i1 / paddle.i1e API rfc doc, update paddle.i0 / paddle.i0e API rfc doc. 【Hackathon 4th No.20&No.21】Add paddle.i1 / paddle.i1e API rfc doc, update paddle.i0 / paddle.i0e API rfc doc. Mar 24, 2023
@zhengqiwen1997
Copy link

为何要更改i0/i0e的RFC,使这俩API分开到两个文档?i0/i0e的工作是 @PommesPeter 这位同学的,若你要修改,请说明理由,并且需要 @PommesPeter 审阅。

@LyndonKong
Copy link
Contributor Author

为何要更改i0/i0e的RFC,使这俩API分开到两个文档?i0/i0e的工作是 @PommesPeter 这位同学的,若你要修改,请说明理由,并且需要 @PommesPeter 审阅。

i0和i0e算子作为需要新增cpp实现的算子,backward操作需要自行实现,不再满足直接的调用关系,需要单独进行测试不再适合放在同一个RFC文档中。考虑到Tensorflow的实现和当前Paddle的第三方库依赖中已经存在相关实现,更加简单的方法是前向算子传播中直接调用对应函数,将四个算子合并到一次pr提交里。

@LyndonKong
Copy link
Contributor Author

辛苦@PommesPeter review一下提案,可以商量一下提案的改进

@PommesPeter
Copy link
Contributor

辛苦@PommesPeter review一下提案,可以商量一下提案的改进

该方案主要特点是什么呢?实现逻辑是差不多的,主要可以考虑代码结构的优化,如果 Eigen 提供相关的 API 确实很好,但自己编写问题应该也不是很大,我这边实现了 i0 和 i0e,你这边可以把 i1 和 i1e 实现,这样我们两边联调一下。

@LyndonKong
Copy link
Contributor Author

辛苦@PommesPeter review一下提案,可以商量一下提案的改进

该方案主要特点是什么呢?实现逻辑是差不多的,主要可以考虑代码结构的优化,如果 Eigen 提供相关的 API 确实很好,但自己编写问题应该也不是很大,我这边实现了 i0 和 i0e,你这边可以把 i1 和 i1e 实现,这样我们两边联调一下。

主要是出于代码风格和实现一致性上的考虑,计算精度和性能上是相似的,如果reviewer可以接受两边实现参考的不一致我这边也是没有问题的。

@zhengqiwen1997
Copy link

因为这两个任务是分开的,建议两位各自提一个PR来进行编码。至于代码风格和实现一致性,我认为两位可以统一一下,讨论下用哪种更好(从性能、计算精度和代码可读性来考虑),统一后相关文档也要进行改进。同时我认为RFC文档还是一个任务写一个文档为好,即 @PommesPeter 同学那样写。

@PommesPeter
Copy link
Contributor

PommesPeter commented Mar 29, 2023

我认为从代码风格的角度可以考虑 @LyndonKong 同学的提案,引入 Eigen 库的方式代码实现上更加简洁和逻辑清晰,我这边可以修改一下先前的 rfc 提案,同学只需要把 i1 和 i1e 的提案放上即可。从代码可读性和计算精度而言,引入 Eigen 库能够对齐目前主流框架,对算子本身比较好。对于编码实现可以均采用引入 Eigen 库的方式。我认为可以这样统一:

  1. 我这边参考 Eigen3 库的实现,将已有实现进行迁移,转变使用调用 Eigen3 库的方式。
  2. 对于代码对齐,我们可以分两步,第一次 pr 先提交前向传播的算子,第二次提交反向传播的算子。

reviewer 对于上述方案是否可以考虑?这样能保证代码风格统一且有效。

@zhengqiwen1997
Copy link

好的,没问题!那么请 @PommesPeter 同学修改i0/i0e的RFC文档和代码PR,@LyndonKong 同学修改这个文档,将i1/i1e放入一个文档中。第一次 pr 先提交前向传播的算子,代码对齐后再进行反向传播的算子编写。

@LyndonKong
Copy link
Contributor Author

好的,我将会关闭这个pr重新提交

@paddle-bot
Copy link

paddle-bot bot commented Mar 29, 2023

很抱歉,经过我们的反复讨论,你的PR暂未达到合入标准,请阅读飞桨社区贡献指南,你可以重新提交新的PR,我们先将此PR关闭,感谢你的贡献。
Sorry to inform you that through our discussion, your PR fails to meet the merging standard (Reference: Paddle API Design Standard 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants