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.29】add RFC for PixelUnshuffle #43

Closed
wants to merge 4 commits into from

Conversation

thunder95
Copy link
Contributor

增加paddle.nn. PixelUnshuffle设计文档。

@CLAassistant
Copy link

CLAassistant commented Mar 17, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Your Name
❌ thunder95


Your Name seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@thunder95
Copy link
Contributor Author

already signed

@paddle-bot-old
Copy link

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

@shiyutang shiyutang left a comment

Choose a reason for hiding this comment

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

方案整体完备,但是实现细节等需要步骤化说明,请修改方案后再提交代码。修改示例参照:https://github.com/PaddlePaddle/community/blob/master/rfcs/APIs/20200301_api_design_for_quantile.md

return 'downscale_factor={}'.format(self.downscale_factor)
```

其中的`F.channel_shuffle`是由C++实现的,主要代码为:
Copy link
Contributor

Choose a reason for hiding this comment

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

这部分有笔误

Comment on lines 76 to 79
}
```


Copy link
Contributor

Choose a reason for hiding this comment

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

这部分需要增加tf的实现逻辑和两个方案的逻辑步骤化描述

Comment on lines 80 to 82
# 四、对比分析
无论是C++实现还是组合API实现,其逻辑都是十分简单的,故考虑使用C++编写新的算子以期取得更高的效率。

Copy link
Contributor

Choose a reason for hiding this comment

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

需要进行方案优劣对比分析

Comment on lines +94 to +97
## API实现方案

参考`paddle.nn.PixelShuffle`来实现`paddle.nn.PixelUnshuffle`,顺便实现`paddle.nn.functional.pixel_unshuffle`。

Copy link
Contributor

Choose a reason for hiding this comment

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

需要对文件存放位置和具体逻辑进行描述

Comment on lines 100 to 103
考虑测试的情况:
- 与PyTorch的结果的一致性;
- 反向传播的正确性;
- 错误检查:`downscale_factor`不合法或不整除通道数时能正确抛出异常。
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.

已完成上述所有修改建议

已完成下列修改
1. 修改笔误
2. 增加tf的实现逻辑和两个方案的逻辑步骤化描述.
3. 进行方案优劣对比分析
4. 测试情况需要丰富化,
@paddle-bot-old
Copy link

PR格式检查通过,你的PR将接受Paddle专家以及开源社区的review,请及时关注PR动态。
The format inspection passed. Your PR will be reviewed by experts of Paddle and developers from the open-source community. Stay tuned.

@paddle-bot-old
Copy link

你的PR有最新反馈,请及时修改。
There’s the latest feedback about your PR. Please check.

@dingjiaweiww
Copy link
Contributor

你好,我们发现你的设计文档中部分内容与另一个开发者的pr(#44)有雷同现象,pr(#44) 先于你提交,从遵守开源规则、维护有序开源生态的角度出发,你可以基于其他开发者的pr 进行commit修改,不建议再次提交个人pr,因此我们会优先review pr(#44),感谢你对飞桨的贡献!

已完成下列修改
1. 修改笔误
2. 增加tf的实现逻辑和两个方案的逻辑步骤化描述.
3. 进行方案优劣对比分析
4. 测试情况需要丰富化,
@TCChenlong TCChenlong closed this Apr 12, 2022
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.

7 participants