-
Notifications
You must be signed in to change notification settings - Fork 5.9k
【Hackathon 9th Sprint No.06】NO.06 paddle.nn.MaxPool2D兼容性增强 #76714
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
base: develop
Are you sure you want to change the base?
【Hackathon 9th Sprint No.06】NO.06 paddle.nn.MaxPool2D兼容性增强 #76714
Conversation
|
你的PR提交成功,感谢你对开源项目的贡献! |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (80.25%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #76714 +/- ##
==========================================
Coverage ? 80.25%
==========================================
Files ? 11
Lines ? 476
Branches ? 0
==========================================
Hits ? 382
Misses ? 94
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/re-run all-failed |
|
#76457 这个PR还需要吗 |
zhwesky2010
left a comment
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,确保增加的逻辑都能测试到,同时在PaConvert里也加一些dilation的测试,确保与torch结果也是对齐的。
| output = _C_ops.max_pool2d_with_index( | ||
| x, kernel_size, stride, padding, False, False, ceil_mode | ||
| ) | ||
| if dilation_greater_than_one: |
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.
能否直接在原来的max_pool2d_with_index上加参数dilation
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.
这个可以,最新的commit实现了
| op_type = 'max_pool2d_with_index' if return_mask else "pool2d" | ||
| if return_mask: | ||
| if dilation_greater_than_one: | ||
| op_type = 'max_pool2d_with_dilations_and_index' |
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上加参数。这样op会太多了比较复杂
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.
可以在 max_pool2d_with_index 上加参数,但是没有办法在 pool_2d 上加参数。
| backward : max_grad | ||
| interfaces : paddle::dialect::InferSymbolicShapeInterface, paddle::dialect::LayoutTransformationInterface | ||
|
|
||
| - op : max_pool2d_with_dilations |
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有点太多了,只是多一个兼容的参数,还是在原来上改吧
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.
原先的PR是那样做的,会有一大堆兼容性问题,很难在pool_2d上加参数。
| return true; | ||
| } | ||
|
|
||
| bool MaxPool2dWithDilationsGradOpInferSymbolicShape( |
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.
infer_sym这些需要开CINN模式来测
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.
https://github.com/PaddlePaddle/Paddle/blob/develop/test/legacy_test/op_test.py#L2941 ,测这个需要 check_symbol_infer(默认值为True)、check_pir 都为True,check_pir 已经设置为True。
不需要了 |
|
@zhwesky2010 老师有时间 review 一下吧,ci 基本通过了。 |
|
/re-run all-failed |
|
|
||
| # use 2d to implement 1d should expand padding in advance. | ||
| padding = _expand_low_nd_padding(padding) | ||
| dilation = convert_to_list(1, 2, 'pool_dilation') |
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.
这里是一个写死的dilation吗?
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.
这个是 max_pool1d,暂时还不支持 dilation,因为它调用了cpp层的 max_pool2d_with_index,所以这里给一个写死的值做兼容
| "data_format": data_format, | ||
| }, | ||
| ) | ||
| if return_mask: |
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.
return_mask为何影响到dilation是否传入
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.
这个也是 max_pool1d,也是为了兼容cpp层的修改
| ) | ||
|
|
||
| dilation = convert_to_list(dilation, 2, 'pool_dilation') | ||
| dilation_greater_than_one = dilation[0] > 1 or dilation[1] > 1 |
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.
这个>1还得额外搞个分支吗,不能传入到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里再判断:done
| ) | ||
| return output if return_mask else output[0] | ||
| else: | ||
| if dilation_greater_than_one: |
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.
如果help这个分支有问题,可以不修改。这个功能加到dynamic_or_pir下。
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.
python尽量不要去加太多分支和逻辑。pool_2d的兼容性问题是什么?是在老IR分支吗,老IR可以不维护
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.
dilation_greater_than_one 不需要了,最新 commit 将相关分支下沉到 cpp 了
|
test_layers 报错了,我再看看 |
zhwesky2010
left a comment
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.
| ) | ||
| return output if return_mask else output[0] | ||
| else: | ||
| return _C_ops.pool2d( |
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.
是说直接修改pool2d会引起很多兼容性问题对吧,所以改名为一个新op:max_pool2d_with_dilations
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.
avg 等等其他算子也依赖 pool2d,对于它们来说,dilation是冗余的
|
/re-run all-failed |
5 similar comments
|
/re-run all-failed |
|
/re-run all-failed |
|
/re-run all-failed |
|
/re-run all-failed |
|
/re-run all-failed |
|
一条流水线(workflow,比如 CI、CI build)里有任何一个 job 仍在运行的话,rerun 是没效果的,别发了,我这邮件一直响 😂 |
|
目前这个 rerun comment 是写在哪里的文档里了?需要更新下文档,另外 rerun 失败需要通过 comment 来告知开发者 |
我的,不好意思 |
文档的问题,问下 |
这个目前确实是缺少外部文档记录,需要添加一下,之前应该是别人告诉才知道 |
只要发这个指令就会响吗,我等全部都跑完再re-run可以不 |
|
/re-run all-failed |


PR Category
Operator Mechanism
PR Types
New features
Description
paddle.nn.MaxPool2D兼容性增强