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.

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

【Hackathon 5th No.13】【关联 PR】Added uint8&int8&int16 support for compare_kernel -part #58209

Merged
merged 22 commits into from
Nov 23, 2023
Merged

【Hackathon 5th No.13】【关联 PR】Added uint8&int8&int16 support for compare_kernel -part #58209

merged 22 commits into from
Nov 23, 2023

Conversation

jjyaoao
Copy link
Contributor

@jjyaoao jjyaoao commented Oct 18, 2023

PR types

Others

PR changes

Others

Description

reference link: #57882

@paddle-bot
Copy link

paddle-bot bot commented Oct 18, 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.

@paddle-bot paddle-bot bot added the contributor External developers label Oct 18, 2023
@luotao1 luotao1 self-assigned this Oct 19, 2023
@PommesPeter PommesPeter mentioned this pull request Oct 19, 2023
Merged
3 tasks
@jjyaoao jjyaoao force-pushed the jjy37 branch 2 times, most recently from 402595f to 13d8b5c Compare October 20, 2023 03:04
@Ligoml Ligoml mentioned this pull request Oct 20, 2023
Open
@jjyaoao jjyaoao mentioned this pull request Oct 20, 2023
Closed
@jjyaoao jjyaoao force-pushed the jjy37 branch 3 times, most recently from 3d88e7f to 23aa5e6 Compare October 20, 2023 16:42
@jjyaoao jjyaoao force-pushed the jjy37 branch 2 times, most recently from b57ebc1 to c8d4a5b Compare October 21, 2023 11:07
@paddle-ci-bot
Copy link

paddle-ci-bot bot commented Oct 29, 2023

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

Aurelius84
Aurelius84 reviewed Oct 30, 2023
View reviewed changes
paddle/phi/kernels/cpu/compare_kernel.cc
@@ -110,6 +110,7 @@ PD_REGISTER_KERNEL(equal_all,
ALL_LAYOUT, \
phi::func##Kernel, \
bool, \
int8_t, \
Copy link
Contributor

Choose a reason for hiding this comment

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

你这里对所有的compare kernel 都添加了int8_t 类型的注册？但单测里只加了less_than的测试

paddle/phi/kernels/kps/compare_kernel.cu Outdated
kernel->OutputAt(0).SetDataType(phi::DataType::BOOL); \
}

PD_REGISTER_COMPARE_KERNEL(less_than, LessThan)
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_LESS_THAN_KERNEL？若是，这里其实不需要宏定义，直接使用PD_REGISTER_KERNEL即可，因为你这样写并没有节省code

@jjyaoao jjyaoao force-pushed the jjy37 branch 2 times, most recently from bafbabe to f22b939 Compare November 2, 2023 13:45
@jjyaoao jjyaoao changed the title 【Hackathon 5th No.13】【关联 PR】Added int8 support for less_than 【Hackathon 5th No.13】【关联 PR】Added int8&int16 support for compate_kernel Nov 2, 2023
@PaddlePaddle PaddlePaddle locked and limited conversation to collaborators Nov 14, 2023
@PaddlePaddle PaddlePaddle unlocked this conversation Nov 14, 2023
luotao1
luotao1 reviewed Nov 17, 2023
View reviewed changes
test/legacy_test/test_compare_op.py
@@ -513,7 +523,7 @@ def test_check_output(self):


class TestCompareOpError(unittest.TestCase):
def test_errors(self):
def test_int16_support(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

这里为什么改名呢？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里为什么改名呢？

当时想的是功能变化了，最开始这里描述的是一旦遇到int16的输出就报错，我把他的内容改为检测是否支持int16的test，我现在调整回来~

@luotao1 luotao1 changed the title 【Hackathon 5th No.13】【关联 PR】Added int8&int16 support for compate_kernel 【Hackathon 5th No.13】【关联 PR】Added int8&int16 support for compate_kernel -part Nov 17, 2023
luotao1
luotao1 reviewed Nov 17, 2023
View reviewed changes
test/legacy_test/test_compare_op.py
op = eval("paddle.%s" % self.op_type)
self.assertRaises(TypeError, op, x=x, y=a)
self.assertRaises(TypeError, op, x=a, y=y)
Copy link
Contributor

Choose a reason for hiding this comment

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

46-51为什么要修改呢，用原来的方式不行么？

Copy link
Contributor Author

@jjyaoao jjyaoao Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

46-51为什么要修改呢，用原来的方式不行么？

是的，如果用原来的就会报类型错误，因为这个单侧本来的意义在于，防止compare的kernel能够使用int16，一旦检测到int16类型后会自动抛异常，对于50和51行而言：
50行检查当 x 是 int32 类型而 y 是 int16 类型时，调用 op 是否会引发错误。
51行检查当 x 是 int16 类型而 y 是 int32 类型时，同样的情况。
然而这个pr的功能本身就是支持int16，所以必须得调整这个地方

Copy link
Contributor

Choose a reason for hiding this comment

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

那前面单测的名字就不应该改回来了 test_int16_support 比较合适

Copy link
Contributor Author

Choose a reason for hiding this comment

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

那前面单测的名字就不应该改回来了 test_int16_support 比较合适

好的好的，最后那个文档风格检测可能是我修改了一部分源码里的注释导致的，可能需要看一看怎么检查一下

luotao1
luotao1 previously approved these changes Nov 20, 2023
View reviewed changes
@luotao1 luotao1 changed the title 【Hackathon 5th No.13】【关联 PR】Added int8&int16 support for compate_kernel -part 【Hackathon 5th No.13】【关联 PR】Added int8&int16 support for compare_kernel -part Nov 21, 2023
@jeff41404
Copy link
Contributor

jeff41404 commented Nov 22, 2023 •
edited
Loading

shall we add uint8 together? after adding it, all the types(except complex) are completely supported in compare and logic kernel

@jjyaoao
Copy link
Contributor Author

jjyaoao commented Nov 22, 2023

shall we add uint8 together? after adding it, all the types(except complex) are completely supported in compare and logic kernel

No problem, it’s a good suggestion~

@jjyaoao jjyaoao changed the title 【Hackathon 5th No.13】【关联 PR】Added int8&int16 support for compare_kernel -part 【Hackathon 5th No.13】【关联 PR】Added uint8&int8&int16 support for compare_kernel -part Nov 23, 2023
@jjyaoao
Copy link
Contributor Author

jjyaoao commented Nov 23, 2023

shall we add uint8 together? after adding it, all the types(except complex) are completely supported in compare and logic kernel

Done

jeff41404
jeff41404 approved these changes Nov 23, 2023
View reviewed changes
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
Copy link
Contributor

luotao1 commented Nov 23, 2023

请修改下对应API的中文文档：equal、greater_equal、greater_than、less_equal、less_than、not_equal

@luotao1 luotao1 merged commit d700b4c into PaddlePaddle:develop Nov 23, 2023
27 of 28 checks passed
@jjyaoao
Copy link
Contributor Author

jjyaoao commented Nov 23, 2023

请修改下对应API的中文文档：equal、greater_equal、greater_than、less_equal、less_than、not_equal

收到~，PaddlePaddle/docs#6330

SecretXV pushed a commit to SecretXV/Paddle that referenced this pull request Nov 28, 2023
@jjyaoao @SecretXV
【Hackathon 5th No.13】【关联 PR】Added uint8&int8&int16 support for compar…
a27555f 
…e_kernel -part (PaddlePaddle#58209)

* 【Hackathon 5th No.13】【关联 PR】Added int8 support for less_than

* update 1.0

* update test

* update v1.3

* Update test_compare_op.py

* Update test_compare_op.py

* Update test_compare_op.py

* Update test_compare_op.py,test=document_fix

* Update test_compare_op.py

* Update test_compare_op.py

* Update test_compare_op.py

* Update test_compare_op.py

* Update test_compare_op.py

* Update test_compare_op.py

* Update compare_kernel.cc

* Update compare_kernel.cu

* Update compare_kernel.cc

* Update compare_kernel.cu

* Update test_compare_op.py

* Update logic.py

* Update compare_kernel.cc

* Update compare_kernel.cc
@luotao1 luotao1 mentioned this pull request Jan 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aurelius84 Aurelius84 Aurelius84 left review comments

@jeff41404 jeff41404 jeff41404 approved these changes

@luotao1 luotao1 luotao1 left review comments

Assignees

@luotao1 luotao1

@jeff41404 jeff41404

Labels
contributor External developers PaddlePaddle Hackathon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jjyaoao @luotao1 @jeff41404 @Aurelius84