-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
【Hackathon 4th No.29】为 Paddle 新增 paddle.sparse.slice 稀疏 API #53794
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
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. |
a88e97a
to
74ceb80
Compare
@zkh2016 您好,目前已经修改完毕,麻烦再重新Review以下 |
const phi::IntArray& axes_arr, | ||
const phi::IntArray& starts_arr, | ||
const phi::IntArray& ends_arr, |
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.
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?
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.
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:
Paddle/paddle/phi/kernels/impl/slice_kernel_impl.h
Lines 102 to 114 in d1d43c2
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(); |
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.
Additionally, the configuration of DenseTensor's slice API in legacy_ops.yaml
is:
Paddle/paddle/phi/api/yaml/legacy_ops.yaml
Lines 906 to 913 in cdbf62f
- 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 |
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.
The signature of the SliceKernel should mainly refers to
Paddle/paddle/phi/kernels/slice_kernel.h
Lines 25 to 32 in d1d43c2
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
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.
Thanks for your reply! I will propose a PR to correct it when I have free time.
@jeff41404 Thank you again for your review. Finally, it might be better to use Please see the latest commit for details and review it again! |
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 for docs
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 types
New features
PR changes
APIs
Description
实现第四期 Hackathon 第 29 个任务
中文文档: PaddlePaddle/docs#5895
原始 RFC: PaddlePaddle/community#382
原始 RFC 的修复: PaddlePaddle/community#539