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

[Phi] Migrate unfold_op into phi #39778

Merged
merged 5 commits into from
Feb 22, 2022

Conversation

Aurelius84
Copy link
Contributor

PR types

Others

PR changes

OPs

Describe

[Phi] Migrate unfold_op into phi

@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@chenwhql chenwhql requested a review from zyfncg February 22, 2022 06:03
PADDLE_ENFORCE_GT(
output_height,
0,
phi::errors::InvalidArgument(
Copy link
Contributor

Choose a reason for hiding this comment

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

一个讨论,话说其实正在phi里面,我们不用加这个phi的前缀,errors还有make_ddim,后面是不是统一把这些去掉让代码更简洁一些

namespace funcs {

//////// CalcOutputSize Functor ///////
inline int CalcOutputSize(int input_size,
Copy link
Contributor Author

@Aurelius84 Aurelius84 Feb 22, 2022

Choose a reason for hiding this comment

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

common_shape.h的函数是多个Op公有的?这个CalcOutputSize函数是unfold单独用到的。另外后续common_shape.h 后续如果包含了超多函数,这里include会不会代码膨胀?

namespace funcs {

//////// CalcOutputSize Functor ///////
inline int CalcOutputSize(int input_size,
Copy link
Contributor Author

@Aurelius84 Aurelius84 Feb 22, 2022

Choose a reason for hiding this comment

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

common_shape.h的函数是多个Op公有的?这个CalcOutputSize函数是unfold单独用到的。另外后续common_shape.h 后续如果包含了超多函数,这里include会不会代码膨胀?

Copy link
Contributor

@zyfncg zyfncg left a comment

Choose a reason for hiding this comment

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

LGTM

namespace phi {

template <typename T, typename Context>
void UnfoldGradKernel(const Context& ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

目前kernel的Context模板参数变量名建议使用dev_ctx

namespace funcs {

//////// CalcOutputSize Functor ///////
inline int CalcOutputSize(int input_size,
Copy link
Contributor

Choose a reason for hiding this comment

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

如果这个函数只有unfold这个op使用的话,可以放到impl下,funcs下一般放的是多op使用的函数

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我在 #39796 这里会移动到impl目录里

@Aurelius84 Aurelius84 merged commit 1aa6777 into PaddlePaddle:develop Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants