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 4th No.29】为 Paddle 新增 paddle.sparse.slice 稀疏 API #53794

Merged
merged 24 commits into from
Jun 3, 2023

Conversation

ScottWong98
Copy link
Contributor

@ScottWong98 ScottWong98 commented May 14, 2023

PR types

New features

PR changes

APIs

Description

实现第四期 Hackathon 第 29 个任务

中文文档: PaddlePaddle/docs#5895

原始 RFC: PaddlePaddle/community#382

原始 RFC 的修复: PaddlePaddle/community#539

@paddle-bot
Copy link

paddle-bot bot commented May 14, 2023

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@ScottWong98 ScottWong98 marked this pull request as draft May 14, 2023 16:44
@paddle-bot paddle-bot bot added the contributor External developers label May 14, 2023
@ScottWong98 ScottWong98 marked this pull request as ready for review May 20, 2023 11:18
@zhwesky2010 zhwesky2010 requested a review from zkh2016 May 26, 2023 06:11
@paddle-ci-bot
Copy link

paddle-ci-bot bot commented May 29, 2023

Sorry to inform you that 428dc2e's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

paddle/phi/kernels/sparse/gpu/slice_kernel.cu Show resolved Hide resolved
paddle/phi/kernels/sparse/gpu/slice_kernel.cu Outdated Show resolved Hide resolved
paddle/phi/kernels/sparse/gpu/slice_kernel.cu Outdated Show resolved Hide resolved
paddle/phi/kernels/sparse/gpu/slice_kernel.cu Outdated Show resolved Hide resolved
paddle/phi/kernels/sparse/gpu/slice_kernel.cu Outdated Show resolved Hide resolved
paddle/phi/kernels/sparse/gpu/slice_kernel.cu Show resolved Hide resolved
paddle/phi/kernels/sparse/gpu/slice_kernel.cu Outdated Show resolved Hide resolved
@ScottWong98
Copy link
Contributor Author

@zkh2016 您好,目前已经修改完毕,麻烦再重新Review以下

zkh2016
zkh2016 previously approved these changes Jun 1, 2023
Comment on lines 231 to 233
const phi::IntArray& axes_arr,
const phi::IntArray& starts_arr,
const phi::IntArray& ends_arr,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use parameter name of axes_arr, starts_arr and ends_arr? No other kernel are not named like this, just using axes, starts and ends is better?

Copy link
Contributor Author

@ScottWong98 ScottWong98 Jun 2, 2023

Choose a reason for hiding this comment

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

Thank you very much for your review!

The reason is that the data type of these parameters is IntArray, and I will get their underlying data in implementing the function, like axes_arr.GetData(). For convenience, I use axes_arr as the parameter's name and axes as the variable name indicating the underlying data described above in the function body.

And I found that the names of the starts and ends parameters are also starts_array and ends_array in DenseTensor's slice function:

template <typename T, typename Context>
void SliceKernel(const Context& ctx,
const DenseTensor& input,
const std::vector<int64_t>& axes,
const IntArray& starts_arr,
const IntArray& ends_arr,
const std::vector<int64_t>& infer_flags,
const std::vector<int64_t>& decrease_axis,
DenseTensor* out) {
int rank = input.dims().size();
auto& starts = starts_arr.GetData();
auto& ends = ends_arr.GetData();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, the configuration of DenseTensor's slice API in legacy_ops.yaml is:

- op : slice
args : (Tensor input, int64_t[] axes, IntArray starts, IntArray ends, int64_t[] infer_flags, int64_t[] decrease_axis)
output : Tensor
infer_meta :
func : SliceRawInferMeta
kernel :
func : slice
backward : slice_grad

Copy link
Contributor

Choose a reason for hiding this comment

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

The signature of the SliceKernel should mainly refers to

void SliceKernel(const Context& ctx,
const DenseTensor& input,
const std::vector<int64_t>& axes,
const IntArray& starts,
const IntArray& ends,
const std::vector<int64_t>& infer_flags,
const std::vector<int64_t>& decrease_axis,
DenseTensor* out);

The function mentioned above in Paddle/paddle/phi/kernels/impl/slice_kernel_impl.h is its implementation, and it's better to be the same, If you are interested, can propose another PR to correct the parameter names in slice_kernel_impl.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your reply! I will propose a PR to correct it when I have free time.

@ScottWong98
Copy link
Contributor Author

@jeff41404 Thank you again for your review.

Finally, it might be better to use axes, starts, and ends as parameter names. And I have changed axes_arr, starts_arr, and ends_arr to axes, starts, and ends.

Please see the latest commit for details and review it again!

Copy link
Contributor

@sunzhongkai588 sunzhongkai588 left a comment

Choose a reason for hiding this comment

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

LGTM for docs

Copy link
Contributor

@jeff41404 jeff41404 left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1 luotao1 merged commit d71baff into PaddlePaddle:develop Jun 3, 2023
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.

None yet

6 participants