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 4th No.10] 为 paddle 新增 paddle.copysign API #367

Merged
merged 9 commits into from
May 11, 2023
Merged

[Hackathon 4th No.10] 为 paddle 新增 paddle.copysign API #367

merged 9 commits into from
May 11, 2023

Conversation

Ainavo
Copy link
Contributor

@Ainavo Ainavo commented Feb 21, 2023 •
edited
Loading

@paddle-bot
Copy link

paddle-bot bot commented Feb 21, 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.

@Ainavo Ainavo mentioned this pull request Feb 21, 2023
Closed
zoooo0820
zoooo0820 reviewed Feb 23, 2023
View reviewed changes
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.

你好，非常感谢反馈-0处的行为问题。
从文档的调研部分来看，除下列列出的细节小问题外，有两个问题需要特别提出：

  • 在-0时的表现问题，如果等到支持signbit以后再改回来适配，属于不兼容升级，这是需要尽力避免的情况
  • copysign在Pytorch中是存在kernel的，可以再补充调研一下，底层实现逻辑不算太复杂。

综合考虑，这个API是否通过底层kernel的实现更为合适

rfcs/APIs/20230221_api_design_for_copysign.md Outdated Show resolved Hide resolved
rfcs/APIs/20230221_api_design_for_copysign.md Outdated Show resolved Hide resolved
rfcs/APIs/20230221_api_design_for_copysign.md Outdated Show resolved Hide resolved
rfcs/APIs/20230221_api_design_for_copysign.md Outdated Show resolved Hide resolved
rfcs/APIs/20230221_api_design_for_copysign.md Outdated Show resolved Hide resolved
rfcs/APIs/20230221_api_design_for_copysign.md Outdated

代码如下：

Pytorch 中具体使用 `python` 实现，[代码](https://cs.github.com/pytorch/pytorch/blob/4d753b50451607b3314f827993df7e5527f0c0a7/torch/_refs/__init__.py#L1033)如下：
Copy link
Contributor

Choose a reason for hiding this comment

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

这里需要同时列举下torch中kernel的实现情况 ，如这里

@Ligoml Ligoml mentioned this pull request Mar 7, 2023
Closed
@Ainavo
Copy link
Contributor Author

Ainavo commented Apr 17, 2023

辛苦 @zoooo0820 有时间再 review 一下，不知道对 torch 中 copysign 函数的实现逻辑的描述是否正确，希望能得到修改意见~

@Ainavo Ainavo requested a review from zoooo0820 May 10, 2023 02:40
zoooo0820
zoooo0820 approved these changes May 11, 2023
View reviewed changes
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

@zoooo0820 zoooo0820 merged commit 6de4a14 into PaddlePaddle:master May 11, 2023
@Ainavo Ainavo deleted the rfc_for_copysign branch May 11, 2023 07:08
@Ainavo Ainavo mentioned this pull request May 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zoooo0820 zoooo0820 zoooo0820 approved these changes

Assignees

@luotao1 luotao1

@zoooo0820 zoooo0820

@Ligoml Ligoml

@cloud2009 cloud2009

Labels
contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ainavo @zoooo0820 @luotao1 @Ligoml @cloud2009