Skip to content

Conversation

@NKNaN
Copy link
Contributor

@NKNaN NKNaN commented Oct 14, 2024

PR Category

User Experience

PR Types

New features

Description

rfc: PaddlePaddle/community#968

@paddle-bot
Copy link

paddle-bot bot commented Oct 14, 2024

你的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.

1
"""
if in_dynamic_mode():
Copy link
Contributor

Choose a reason for hiding this comment

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

add data dtype check

Copy link
Contributor Author

@NKNaN NKNaN Oct 23, 2024

Choose a reason for hiding this comment

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

added in branch source is not None


} // namespace phi

PD_REGISTER_KERNEL_FOR_ALL_BACKEND_DTYPE(set, STRIDED, phi::SetInplaceKernel) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

add register for cpu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does PD_REGISTER_KERNEL_FOR_ALL_BACKEND_DTYPE already register for cpu?

Copy link
Contributor

Choose a reason for hiding this comment

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

PD_REGISTER_KERNEL is work for cpu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised

@paddle-ci-bot
Copy link

paddle-ci-bot bot commented Oct 23, 2024

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

raise ValueError(
f"Input (source) should be paddle.Tensor but received {type(source)}"
)
if source.dtype not in [
Copy link
Contributor

Choose a reason for hiding this comment

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

suppose to use check_dtype(), also in python code, "bfloat16" should type with "uint16"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised

int64_t,
float,
double,
phi::dtype::complex<float>,
Copy link
Contributor

Choose a reason for hiding this comment

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

is CPU unspport float16 and bfloat16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know whether all kind of CPU support float16 and bfloat16 so they are not registered here

Copy link
Contributor

@zxcd zxcd Oct 31, 2024

Choose a reason for hiding this comment

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

I think you can try the same way with paddle/phi/kernels/strided_slice_kernel.cc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, float16 and bfloat16 are added.

Copy link
Contributor

Choose a reason for hiding this comment

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

STRIDED may change to ALL_LAYOUT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised

Copy link
Contributor

@zxcd zxcd left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -0,0 +1,77 @@
// Copyright (c) 2024 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

@jeff41404 jeff41404 Nov 4, 2024

Choose a reason for hiding this comment

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

inplace is a concept in the level of ops(in the ops.yaml), representing the space where ops's output can write to the space of input. so in the level of kernel, there is no concept of inplace, because both inplace ops and non-inplace ops use the same computational logic and the same kernel.
so, we suggest filename is set_kernel.cc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised

@@ -0,0 +1,30 @@
// Copyright (c) 2024 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

inplace is a concept in the level of ops(in the ops.yaml), representing the space where ops's output can write to the space of input. so in the level of kernel, there is no concept of inplace, because both inplace ops and non-inplace ops use the same computational logic and the same kernel.
so, we suggest filename is set_kernel.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.

revised

'diagonal_scatter',
'combinations',
'signbit',
'set_',
Copy link
Contributor

@jeff41404 jeff41404 Nov 4, 2024

Choose a reason for hiding this comment

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

Do we need to support the usage of paddle.set_(x, source, ....)? If it is not recommended for users to use it this way(recommended only x.set_(source, ...) when x is Tensor), there is no need to add it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This time only support Tensor.set_ , this line can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

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

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

@luotao1 luotao1 merged commit 4ddd410 into PaddlePaddle:develop Nov 6, 2024
28 checks passed
@luotao1
Copy link
Contributor

luotao1 commented Nov 6, 2024

可以提交中文文档啦

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.

5 participants