CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
【Hackathon 6th No.28】为 paddle.round 进行功能增强 -part #64436
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
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
需要在paddle/phi/api/yaml/op_version.yaml 注明对此次的OP add_attr 增加一个属性的修改
@@ -30,7 +30,6 @@ | |||
'rsqrt_', | |||
'ceil_', | |||
'floor_', | |||
'round_', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里删除的原因是
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -295,7 +307,7 @@ PD_REGISTER_KERNEL(log1p, | |||
phi::dtype::complex<double>) {} | |||
|
|||
PD_REGISTER_ACTIVATION_KERNEL_WITH_COMPLEX(hardswish, HardSwishKernel) | |||
PD_REGISTER_ACTIVATION_KERNEL(round, RoundKernel) | |||
PD_REGISTER_KERNEL(round, CPU, ALL_LAYOUT, phi::RoundKernel, float, double) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里不需要修改吧,和原来的宏是一模一样的
@@ -301,6 +314,14 @@ PD_REGISTER_ACTIVATION_KERNEL(hardsigmoid, HardSigmoidKernel) | |||
PD_REGISTER_ACTIVATION_KERNEL_WITH_COMPLEX(hardswish, HardSwishKernel) | |||
PD_REGISTER_ACTIVATION_KERNEL(swish, SwishKernel) | |||
PD_REGISTER_ACTIVATION_KERNEL(round, RoundKernel) | |||
// PD_REGISTER_KERNEL(round, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里删掉冗余注释吧
@@ -149,7 +149,16 @@ DEFINE_ONEDNN_ACTIVATION_KERNEL(Sqrt, SqrtOneDNNFunctor) | |||
DEFINE_ONEDNN_ACTIVATION_KERNEL(Tanh, TanhOneDNNFunctor) | |||
|
|||
// round eltwise primitive doesn't support BF16, nor does it support grad | |||
DEFINE_ONEDNN_ACTIVATION_KERNEL(Round, RoundOneDNNFunctor) | |||
// DEFINE_ONEDNN_ACTIVATION_KERNEL(Round, RoundOneDNNFunctor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里看起来删掉了原来的kernel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个下面通过手动定义了新的kernel,没有使用这个宏。
然后onednn中round应该也是不支持decimal的,所以目前这个pr中onednn版本的round并不支持decimal,只是添加了这个参数,为了编译通过。
目前想到的一个办法是在onednn版本的round中引入其他的onednn算子来实现decimal的逻辑,但这样的话会不会丧失了使用onednn的带来的性能上的提升?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果onednn本身不支持的话,就组合一下其他的算子 组合支持下
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个下面通过手动定义了新的kernel,没有使用这个宏。 然后onednn中round应该也是不支持decimal的,所以目前这个pr中onednn版本的round并不支持decimal,只是添加了这个参数,为了编译通过。 目前想到的一个办法是在onednn版本的round中引入其他的onednn算子来实现decimal的逻辑,但这样的话会不会丧失了使用onednn的带来的性能上的提升?
hi,辛苦按上面这个思路开展吧:在onednn版本的round中引入其他的onednn算子来实现decimal的逻辑
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯嗯,在进行中~
Sorry to inform you that 50570c1's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
@zbt78 麻烦修复一下CI问题 |
@@ -149,7 +149,43 @@ DEFINE_ONEDNN_ACTIVATION_KERNEL(Sqrt, SqrtOneDNNFunctor) | |||
DEFINE_ONEDNN_ACTIVATION_KERNEL(Tanh, TanhOneDNNFunctor) | |||
|
|||
// round eltwise primitive doesn't support BF16, nor does it support grad | |||
DEFINE_ONEDNN_ACTIVATION_KERNEL(Round, RoundOneDNNFunctor) | |||
// DEFINE_ONEDNN_ACTIVATION_KERNEL(Round, RoundOneDNNFunctor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里ONEDNN的kernel进行了注册没
@@ -110,7 +110,7 @@ DEFINE_GPU_ACTIVATION_KERNEL(Rsqrt, CudaRsqrtFunctor) | |||
DEFINE_GPU_ACTIVATION_KERNEL(Softsign, CudaSoftsignFunctor) | |||
DEFINE_GPU_ACTIVATION_KERNEL(Sigmoid, CudaSigmoidFunctor) | |||
DEFINE_GPU_ACTIVATION_KERNEL(LogSigmoid, CudaLogSigmoidFunctor) | |||
DEFINE_GPU_ACTIVATION_KERNEL(Round, CudaRoundFunctor) | |||
// DEFINE_GPU_ACTIVATION_KERNEL(Round, CudaRoundFunctor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里GPU的kernel进行了注册没
看到了很多屏蔽掉注册的,这些kernel注册的修改是否合理
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是注册了的。这里定义kernel时因为增加了参数,原先的宏不适用了,在下面单独定义了。
另外目前在onednn方面实现上还有一点问题导致CI未通过,在修改中。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是注册了的。这里定义kernel时因为增加了参数,原先的宏不适用了,在下面单独定义了。 另外目前在onednn方面实现上还有一点问题导致CI未通过,在修改中。
换了地方注册,就移除原来的宏吧,正式代码里需去掉多余注释
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
换了地方注册kernel,就移除原来的宏吧,正式代码里需去掉多余注释
另外这里没看到GPU、ONEDNN的kernel注册地方?
GPU和ONEDNN的用原先注册的方式就行,不需要修改。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
需要处理下冲突,其他地方应该没问题了,CI通过后approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@zbt78 需要看下 windows- inference流水线,一直都失败 |
这个在本地window编译后进行测试的时候,并没有成功复现CI中的错误,@zhwesky2010 老师可以帮忙看下可能出错的原因吗~ |
可能是CI有些问题,我先rerun一下 |
Sorry to inform you that 8f361d6's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
template <typename Device, typename X, typename Out> | ||
void operator()(Device d, X x, Out out) const { | ||
out.device(d) = x.round(); | ||
float ten_pow_deciamls = std::pow(10, decimals); | ||
out.device(d) = (x * static_cast<T>(ten_pow_deciamls)).round() * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
windows上精度出了问题,我推测是这里虽然公式上与原来一致,但是在计算机内部会有细微的差异经过多种计算重叠,被放大了误差
float ten_pow_deciamls = std::pow(10, decimals); out.device(d) = (x * static_cast<T>(ten_pow_deciamls)).round() * static_cast<T>(1.0 / ten_pow_deciamls);
但是std::pow有细微的精度误差,经过后续一系列的 乘法、round、乘法、倒数,可能细微的精度误差进一步放大。
所以我建议要简化一下公式,不要完全按数学上来写:
对于decimals=0、1、2、3等常见值,直接取计算后的结果?比如0就是out.device(d) = x
@@ -3786,10 +3786,11 @@ | |||
interfaces : paddle::dialect::InferSymbolicShapeInterface | |||
|
|||
- op : round | |||
args : (Tensor x) | |||
args : (Tensor x, int decimals = 0 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
op_version.yaml里面需要写一下add_attr,增加一个参数decimals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
收到!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
请提交对应中文文档的修改 |
PR Category
User Experience
PR Types
New features
Description
torch.round 支持 decimals 参数,表示舍入的小数点位数,paddle 不支持,增加该参数