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

[Paddle Tensor No.6][BUPT] 新增 Tensor.__ror__ #69463

Merged
merged 25 commits into from
Nov 25, 2024

Conversation

ZHOU05030
Copy link
Contributor

@ZHOU05030 ZHOU05030 commented Nov 18, 2024

PR Category

User Experience

PR Types

New features

Description

Paddle Tensor 规范化:新增 Tensor.__ror__,复用已有接口 Tensor.__or__

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
@ZHOU05030 ZHOU05030 changed the title add tensor.ror [Paddle Tensor No.6] 新增 Tensor.__ror__ Nov 18, 2024
@ZHOU05030 ZHOU05030 changed the title [Paddle Tensor No.6] 新增 Tensor.__ror__ [Paddle Tensor No.6][BUPT] 新增 Tensor.__ror__ Nov 18, 2024
@HydrogenSulfate HydrogenSulfate changed the title [Paddle Tensor No.6][BUPT] 新增 Tensor.__ror__ [Paddle Tensor No.6][BUPT] 新增 Tensor.__ror__ Nov 18, 2024
Comment on lines 254 to 255
bitwise_ror,
bitwise_ror_,
Copy link
Contributor

Choose a reason for hiding this comment

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

__ror__bitwise_or实现,不需要新建一个叫做bitwise_ror的公开API,名字可以叫__ror__

Comment on lines 116 to 117
bitwise_ror,
bitwise_ror_,
Copy link
Contributor

Choose a reason for hiding this comment

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

__ror__bitwise_or实现,不需要新建一个叫做bitwise_ror的公开API,名字可以叫__ror__

Comment on lines 737 to 738
'bitwise_ror',
'bitwise_ror_',
Copy link
Contributor

Choose a reason for hiding this comment

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

__ror__bitwise_or实现,不需要新建一个叫做bitwise_ror的公开API,名字可以叫__ror__

@@ -859,6 +863,7 @@
magic_method_func = [
('__and__', 'bitwise_and'),
('__or__', 'bitwise_or'),
('__ror__', 'bitwise_ror'),
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

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.

感谢指正,那这个__ror__函数我应该在哪个文件里实现呢?

Copy link
Contributor

Choose a reason for hiding this comment

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

@HydrogenSulfate
Copy link
Contributor

提交代码之前请先安装 pre-commit:
image

@luotao1 luotao1 added the HappyOpenSource Pro 进阶版快乐开源活动,更具挑战性的任务 label Nov 19, 2024
def __ror__(
x: Tensor, y: Tensor, out: Tensor | None = None, name: str | None = None
) -> Tensor:
return bitwise_or(y, x)
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

Choose a reason for hiding this comment

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

这个文件能加一个动态图的测试吗?期望是 np.ndarray | Tensor 或者 int | Tensor,来触发 __ror__,参考:https://github.com/PaddlePaddle/Paddle/pull/69348/files#diff-60c4d2d54c7500c4405914e9c50c03203923aa4fb48bba1e68d0362be2377ca1

Comment on lines 1296 to 1297
if isinstance(y, int):
y = paddle.to_tensor(y, dtype=x.dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

此处是否需要考虑布尔类型?

@@ -230,6 +230,13 @@ def test_bitwise_or(self):
paddle.to_tensor(x_np), paddle.to_tensor(y_np)
)
res_np_d = x_np.__or__(y_np)
res_np_e = x_np.__or__(y_np)
Copy link
Contributor

Choose a reason for hiding this comment

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

这里建议用x_np.__ror__(y_np)

Comment on lines 234 to 235
tensor_x = paddle.to_tensor(x_np)
tensor_y = paddle.to_tensor(y_np)
Copy link
Contributor

Choose a reason for hiding this comment

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

x应该是python的内置类型(int/bool),y是Tensor,这样才能触发y.__ror__,否则触发的是x.__or__

Copy link
Contributor

Choose a reason for hiding this comment

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

单测修改一下,感觉没测试到预期的代码上

return bitwise_or(y, x, out=out, name=name)
else:
raise TypeError(
f"unsupported operand type(s) for |: '{type(y).__name__}' and 'Tensor' ."
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -239,14 +240,26 @@ def test_bitwise_or(self):
b = x | y
c = x.bitwise_or(y)
d = x.__or__(y)
(b_np, c_np, d_np) = exe.run(
e = x.__ror__(y)
Copy link
Contributor

Choose a reason for hiding this comment

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

此处是不是应该用y.__ror__(x)

Comment on lines 254 to 263
def test_dygraph_ror(self):
paddle.disable_static()
x_int = 5
y_np = np.random.randint(0, 2, [2, 3, 5]).astype("int32")
y_tensor = paddle.to_tensor(y_np)
res_ror = x_int | y_tensor
res_py = x_int | (y_tensor.numpy())
np.testing.assert_array_equal(res_py, res_ror)
paddle.enable_static()

Copy link
Contributor

Choose a reason for hiding this comment

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

其余类型也都测一下,包括应该允许运算的int32/64、bool,以及不允许运算的float32/64、complex64/128,单测参考:https://github.com/PaddlePaddle/Paddle/pull/69348/files#diff-60c4d2d54c7500c4405914e9c50c03203923aa4fb48bba1e68d0362be2377ca1R493-R587

Copy link
Contributor

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.py 也需要添加单测吧?

out: Tensor | None = None,
name: str | None = None,
) -> Tensor:
if paddle.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.

这句话不用吧?动静态图应该都要适配的

Comment on lines 336 to 356
@prog_scope()
def test_ror(self):
x_np = np.random.randint(-100, 100, [2, 3, 5]).astype("int32")
y_np = np.random.randint(-100, 100, [2, 3, 5]).astype("int32")
out_np = x_np | y_np

x = paddle.static.data(name="x", shape=[2, 3, 5], dtype="int32")
y = paddle.static.data(name="y", shape=[2, 3, 5], dtype="int32")
z = x | y
out_ror = y.__ror__(x)

exe = base.Executor()
exe = base.Executor()
out = exe.run(
base.default_main_program(),
feed={"x": x_np, "y": y_np},
fetch_list=[z, out_ror],
)
np.testing.assert_array_equal(out[0], out_np)
np.testing.assert_array_equal(out[1], out_np)

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 1302 to 1303
elif isinstance(y, paddle.base.libpaddle.pir.Value):
return bitwise_or(y, x, out=out, name=name)
Copy link
Contributor

Choose a reason for hiding this comment

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

这个pir.Value分支应该是不需要的吧?如果pir.Value应该直接能触发y的__or__

)
np.testing.assert_array_equal(res_np_b, b_np)
np.testing.assert_array_equal(res_np_c, c_np)
np.testing.assert_array_equal(res_np_d, d_np)
np.testing.assert_array_equal(res_np_e, e_np)

def test_dygraph_ror(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

参考同文件的其它代码,添加pir的测试

z = x_int | y
exe = paddle.static.Executor(place)
out = exe.run(
feed={'x': x_int, 'y': y_np},
Copy link
Contributor

Choose a reason for hiding this comment

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

feed里的'x': x_int应该是不需要的吧?可以确认下

x_bool = True
res_ror_bool = x_bool | y
out_bool = exe.run(
feed={'x': x_bool, 'y': y_np},
Copy link
Contributor

Choose a reason for hiding this comment

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

同上,'x': x_bool应该可以删掉,因为这只是一个int,没有对应的Variable占位符

Comment on lines 271 to 303
paddle.enable_static()
with paddle.pir_utils.IrGuard():
main_program, exe, program_guard = new_program()
with program_guard:
x_int = 5
y_np = np.random.randint(-100, 100, [2, 3, 5]).astype("int32")
y = paddle.static.data("y", y_np.shape, dtype=y_np.dtype)
z = x_int | y
out = exe.run(
main_program,
feed={'x': x_int, 'y': y_np},
fetch_list=[z],
)
out_ref = x_int | y_np
np.testing.assert_array_equal(out[0], out_ref)
x_bool = True
res_ror_bool = x_bool | y
out_bool = exe.run(
main_program,
feed={'x': x_bool, 'y': y_np},
fetch_list=[res_ror_bool],
)
res_py_bool = x_bool | y_np
np.testing.assert_array_equal(out_bool[0], res_py_bool)

for x_invalid in (
np.float32(5.0),
np.float64(5.0),
np.complex64(5),
np.complex128(5.0 + 2j),
):
with self.assertRaises(TypeError):
x_invalid | y
Copy link
Contributor

Choose a reason for hiding this comment

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

pir的单测不要写到test_dygraph_ror里,可以新建一个函数

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.

python/paddle/tensor/tensor.prototype.pyi 这里再补充一下

@@ -1290,6 +1290,21 @@ def bitwise_or(
)


def __ror__(
x: Tensor,
y: Tensor | 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.

y不会是Tensor

@HydrogenSulfate
Copy link
Contributor

提交代码请前请使用pre-commit或者手动修改一下import的顺序:
image

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers HappyOpenSource Pro 进阶版快乐开源活动,更具挑战性的任务
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants