Skip to content

【2.0 API】Enhance affine grid operator #26385

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
Merged
merged 7 commits into from
Aug 25, 2020
Merged

【2.0 API】Enhance affine grid operator #26385

merged 7 commits into from
Aug 25, 2020

Conversation

@wanghaoshuang
Copy link
Contributor

PR types

Others

PR changes

OPs

Describe

  1. Add cuda kernel
  2. Add align corners options
@wanghaoshuang
Enhance affine grid operator:
a949914 
1. Add cuda kernel 2. Add align corners options test=develop
@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.
@wanghaoshuang
Copy link
Contributor Author

wanghaoshuang commented Aug 18, 2020 •
edited
Loading

问题一、能否将对3D的支持推迟一下？

  1. 时间紧迫，支持3D引入的未知风险，有可能导致该PR延期合入Paddle2.0
  2. 支持3D的需求相对不是很多，cudnn的实现只支持2D

问题二、还需要为paddle.nn.functional.affine_grid写单测么？

  1. paddle.nn.functional.affine_grid是直接alias自paddle.fluid.layers.affine_grid, 也就是完全一样的实现
willthefrog
willthefrog requested changes Aug 18, 2020
View reviewed changes
@willthefrog
Copy link
Contributor

问题一、能否将对3D的支持推迟一下？

  1. 时间紧迫，支持3D引入的未知风险，有可能导致该PR延期合入Paddle2.0
  2. 支持3D的需求相对不是很多，cudnn的实现只支持2D

问题二、还需要为paddle.nn.functional.affine_grid写单测么？

  1. paddle.nn.functional.affine_grid是直接alias自paddle.fluid.layers.affine_grid, 也就是完全一样的实现
  1. yes, only add 3d support if time permits
  2. a new version should be add in nn.functional, as there are changes to the API.
willthefrog
willthefrog requested changes Aug 19, 2020
View reviewed changes
paddle/fluid/operators/affine_grid_op.cu Outdated
namespace ops = paddle::operators;
REGISTER_OP_CUDA_KERNEL(
affine_grid,
ops::AffineGridOpKernel<paddle::platform::CUDADeviceContext, float>,
Copy link
Contributor

Choose a reason for hiding this comment

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

so the cuda version mostly reused the eigen implementation? some preliminary benchmark would be nice if time permits.
Copy link
Contributor Author

@wanghaoshuang wanghaoshuang Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Added CUDA kernel.
I will give some performance benchmark on CUDNN version and CUDA kernel version in the future.
Currently, it will use CUDNN kernel when align_corners is true and use CUDA kernel when align_corners is false.
willthefrog
willthefrog previously approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@willthefrog willthefrog left a comment

Choose a reason for hiding this comment

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

need subsequent PR to add support for higher dimensions, otherwise LGTM
saxon-zh
saxon-zh previously approved these changes Aug 24, 2020
View reviewed changes
Copy link

@saxon-zh saxon-zh left a comment

Choose a reason for hiding this comment

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

LGTM
@wanghaoshuang wanghaoshuang dismissed stale reviews from saxon-zh and willthefrog via de31410 August 24, 2020 07:03
willthefrog
willthefrog approved these changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@willthefrog willthefrog left a comment

Choose a reason for hiding this comment

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

LGTM
@wanghaoshuang wanghaoshuang merged commit a065a24 into PaddlePaddle:develop Aug 25, 2020
@wanghaoshuang wanghaoshuang deleted the grid_api branch August 25, 2020 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@wanghaoshuang @willthefrog @saxon-zh