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 No.17】为 Paddle 新增 sgn #164

Merged
merged 5 commits into from
Jul 14, 2022

Conversation

peachlcy
Copy link
Contributor

@peachlcy peachlcy commented Jul 5, 2022

add rfc of paddle.sgn

@CLAassistant
Copy link

CLAassistant commented Jul 5, 2022

CLA assistant check
All committers have signed the CLA.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 5, 2022

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

Copy link
Collaborator

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

整体RFC需要补充较多内容:

  1. sgn是sign的复数功能,需要介绍下paddle.sign的情况。
  2. 提到sgn可以用组合API实现,那具体是用哪些API实现?为什么组合起来能实现复数功能?
  3. 业内方案调研里,numpy和tf的情况如何?
  4. 测试case请按照API验收标准来完成
  5. pytorch实现逻辑简单,故直接参考其代码,pytorch是用C++写的,RFC提到用python来组,那是如何直接参考代码呢?

@peachlcy
Copy link
Contributor Author

peachlcy commented Jul 7, 2022

整体RFC需要补充较多内容:

  1. sgn是sign的复数功能,需要介绍下paddle.sign的情况。
  2. 提到sgn可以用组合API实现,那具体是用哪些API实现?为什么组合起来能实现复数功能?
  3. 业内方案调研里,numpy和tf的情况如何?
  4. 测试case请按照API验收标准来完成
  5. pytorch实现逻辑简单,故直接参考其代码,pytorch是用C++写的,RFC提到用python来组,那是如何直接参考代码呢?

好的

@peachlcy
Copy link
Contributor Author

peachlcy commented Jul 7, 2022

@luotao1 已更新

Copy link
Collaborator

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

RFC内容还是不够详细,RFC的意义是对不熟悉的同学,可以直接了解全貌并进行开发。

  1. pytorch/tf的计算公式是一样的,但numpy的计算公式不一样。numpy的原因在官网有写:

image

放上来会更了解来龙去脉,而不是简单的说“其中对于复数的返回并不是我们期望得到的”,那为什么不是我们期望的呢?numpy为什么这样设计呢?
  1. 实现方法里没有写TF的
  2. 全文多处语句需要组织下,标点符号有的有,有的没有。相关API和代码实现部分可以附上链接。

rfcs/APIs/20220705_api_design_for_sgn.md Outdated Show resolved Hide resolved
rfcs/APIs/20220705_api_design_for_sgn.md Outdated Show resolved Hide resolved
rfcs/APIs/20220705_api_design_for_sgn.md Outdated Show resolved Hide resolved
@peachlcy
Copy link
Contributor Author

peachlcy commented Jul 8, 2022

已按建议更新,但是在验收部分我参考其他已通过的rfc如:
image
image
image
这些较为简单的API在验收部分写的都比较简略,希望您关于此部分有更详细的建议可以提出,便于用来参考修改

@@ -74,10 +78,31 @@ inline c10::complex<T> sgn_impl (c10::complex<T> z) {
}

```

github链接为:https://github.com/pytorch/pytorch/blob/d7fc864f0da461512fb7b972f04e24e296bd266d/aten/src/ATen/native/cpu/zmath.h
156-163
Copy link
Collaborator

Choose a reason for hiding this comment

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

dtype=x.dtype),
name=name)
return gen_math_ops.sign(x, name=name)
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

86-97行的代码格式有点乱,请调整下

如果复数为0,则直接返回0;否则,返回该复数除以它的绝对值的值
对于非复数直接返回其符号
使用is_complex判断输入是否为复数、若为实数则使用sign进行运算;若为复数则使用as_real将其转化为实数tensor,将其中的非零部分除以它自己的绝对值
,最后在使用as_complex将其转换回复数返回。
Copy link
Collaborator

Choose a reason for hiding this comment

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

最后在使用-》最后再使用


# 六、测试和验收的考量

测试考虑的case如下:

- 数值正确性
- 反向
- 异常测试:由于使用了已有API:sign 该API不支持整型运算,仅支持float16, float32 或 float64,所以需要做数据类型的异常测试
- 异常测试:由于使用了已有API:sign该API不支持整型运算,仅支持float16, float32 或 float64,所以需要做数据类型的异常测试
Copy link
Collaborator

Choose a reason for hiding this comment

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

API设计demo中的单测部分写的比较详细,是按照单测开发规范的6个场景逐条来写的。
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

根据您给的建议进行了修改,但是由于目前并未进行具体的代码开发,所以不知道调用的API是否覆盖静态图和GPU测试场景。这部分是开发阶段的工作,可以在后续开发时根据代码修改RFC具体内容。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

请问一下还有需要修改的地方吗?

Copy link
Collaborator

Choose a reason for hiding this comment

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

由于目前并未进行具体的代码开发,所以不知道调用的API是否覆盖静态图和GPU测试场景

虽然没有进行开发,但设计文档中应该是要能预判的:

  • 业内方案里,并没有特殊说明覆盖不了静态图和GPU场景。
  • 这是用组合API形式实现的,单API都能覆盖静态图和GPU测试场景,因此组合的应该也可以。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,已修改

@peachlcy peachlcy requested a review from luotao1 July 12, 2022 09:01
Copy link
Collaborator

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

LGTM

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.

3 participants