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

【SCU】【Paddle Tensor No.5】新增 Tensor.__rand__，复用已有接口Tensor.__and__ #69473

Merged
merged 30 commits into from
Nov 22, 2024
Merged

【SCU】【Paddle Tensor No.5】新增 Tensor.__rand__，复用已有接口Tensor.__and__ #69473

merged 30 commits into from
Nov 22, 2024

Conversation

SCUcookie
Copy link
Contributor

PR Category

User Experience

PR Types

New Features

Description

Paddle Tensor 规范化：新增 Tensor.rand，复用已有接口 Tensor.and

@paddle-bot Paddle-bot
Copy link

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

@paddle-bot paddle-bot bot added the contributor External developers label Nov 18, 2024
@luotao1 luotao1 mentioned this pull request Nov 19, 2024
Closed
@luotao1 luotao1 added the HappyOpenSource Pro 进阶版快乐开源活动，更具挑战性的任务 label Nov 19, 2024
HydrogenSulfate
HydrogenSulfate reviewed Nov 19, 2024
View reviewed changes
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate left a comment

Choose a reason for hiding this comment

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

PR不正确，__rand__的作用是为了支持int & Tensor这种操作，而你的PR只是将__and__复制了一份，并没有实现预期的功能

test/legacy_test/test_math_op_patch_pir.py Outdated
Comment on lines 286 to 288
res_np_c = paddle.bitwise_and(
paddle.to_tensor(y_np), paddle.to_tensor(x_np)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

此处测试应该直接调用__rand__或者通过int & Tensor间接调用__rand__

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

@HydrogenSulfate
Copy link
Contributor

在开发代码之前，可以先通过谷歌了解一下__rxx__系列的函数应该是如何被触发，被调用的: https://www.google.com.hk/search?q=__rand__&newwindow=1&sca_esv=72265b656c89db63&source=hp&ei=5xg8Z9L2FtSk0-kP6PyxuQY&iflsig=AL9hbdgAAAAAZzwm914x_SSucrAZIRCMfG11cCY1IEJA&ved=0ahUKEwiShoKYzOeJAxVU0jQHHWh-LGcQ4dUDCBY&uact=5&oq=__rand__&gs_lp=Egdnd3Mtd2l6IghfX3JhbmRfX0gAUABYAHAAeACQAQCYAQCgAQCqAQC4AQPIAQD4AQKYAgCgAgCYAwCSBwCgBwA&sclient=gws-wiz

@HydrogenSulfate
Copy link
Contributor

image
pre-commit问题

HydrogenSulfate
HydrogenSulfate reviewed Nov 20, 2024
View reviewed changes
test/legacy_test/test_math_op_patch_pir.py Outdated
@@ -269,14 +270,16 @@ def test_bitwise_and(self):
b = x & y
c = x.bitwise_and(y)
d = x.__and__(y)
(b_np, c_np, d_np) = exe.run(
e = x.__rand__(y)
Copy link
Contributor

Choose a reason for hiding this comment

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

这里应该通过 int/bool & Tensor 的方式触发 Tensor.__rand__

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

@SCUcookie
Copy link
Contributor Author

老师我感觉这个逻辑没什么问题呀，为什么ctest不通过呢

@HydrogenSulfate
Copy link
Contributor

老师我感觉这个逻辑没什么问题呀，为什么ctest不通过呢

单测使用 python xxx.py运行，写完代码可以编包安装，再自测一下。

@SCUcookie
Copy link
Contributor Author

老师我感觉这个逻辑没什么问题呀，为什么ctest不通过呢

单测使用 python xxx.py运行，写完代码可以编包安装，再自测一下。

好的

@SCUcookie
Copy link
Contributor Author

image
老师这个bitwise_and第一个tensor第二个int也不支持吗

HydrogenSulfate
HydrogenSulfate reviewed Nov 21, 2024
View reviewed changes
python/paddle/tensor/logic.py Outdated
@@ -1231,6 +1231,11 @@ def bitwise_and(
)


def __rand__(x: Tensor, y: int | bool):
y_tensor = paddle.to_tensor(y, dtype=x.dtype)
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

这里应该有一个判断，只有int、bool类型的才需要to_tensor，再计算and，其他类型的应该抛出错误

raise TypeError(
      f"unsupported operand type(s) for |: '{type(y).__name__}' and 'Tensor'"
  )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已改

HydrogenSulfate
HydrogenSulfate reviewed Nov 22, 2024
View reviewed changes
python/paddle/tensor/logic.py Outdated
@@ -1231,6 +1231,16 @@ def bitwise_and(
)


def __rand__(x: Tensor, y: int | bool):
if not isinstance(y, (int, bool)):
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.

谢谢老师。已修改

test/legacy_test/test_math_op_patch_pir.py
Copy link
Contributor

Choose a reason for hiding this comment

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

还有一个文件需要添加单测：Paddle/test/legacy_test/test_math_op_patch.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

谢谢老师，已修改

@SCUcookie
Copy link
Contributor Author

检测基本完成，麻烦老师看一下

HydrogenSulfate
HydrogenSulfate approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate left a comment

Choose a reason for hiding this comment

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

LGTM

@HydrogenSulfate HydrogenSulfate merged commit 4ff6f10 into PaddlePaddle:develop Nov 22, 2024
27 of 28 checks passed
@SCUcookie SCUcookie mentioned this pull request Dec 3, 2024
Open
74 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HydrogenSulfate HydrogenSulfate HydrogenSulfate approved these changes

Assignees

@luotao1 luotao1

@HydrogenSulfate HydrogenSulfate

Labels
contributor External developers HappyOpenSource Pro 进阶版快乐开源活动，更具挑战性的任务
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SCUcookie @HydrogenSulfate @luotao1