Skip to content

Conversation

zrr1999
Copy link
Member

@zrr1999 zrr1999 commented Jun 10, 2024

PR Category

User Experience

PR Types

Improvements

Description

类型标注:

  • paddle/tensor/array.py

Copy link

paddle-bot bot commented Jun 10, 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 Jun 10, 2024
@zrr1999 zrr1999 requested review from SigureMo and gouzil June 10, 2024 11:59
@zrr1999
Copy link
Member Author

zrr1999 commented Jun 10, 2024

@megemini

@zrr1999 zrr1999 changed the title [Type Hints] add paddle/tensor/array.py [Typing][task 1] Add type annotations for paddle/tensor/array.py Jun 10, 2024
@@ -57,16 +72,16 @@ def array_length(array):
elif in_pir_mode():
if (
not isinstance(array, paddle.pir.Value)
or not array.is_dense_tensor_array_type()
or not array.is_dense_tensor_array_type() # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

我们代码里就不要加 # type: ignore 了吧,不然太多了,只在示例代码,以及我们自己写的代码里加吧,别人写的代码不用管

@megemini 觉得呢

Copy link
Contributor

Choose a reason for hiding this comment

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

我们代码里就不要加 # type: ignore 了吧,不然太多了,只在示例代码,以及我们自己写的代码里加吧,别人写的代码不用管

嗯 ~ 加这个东西是为了过测试 ~ Paddle 本身不需要 mypy 检查,暂时也就没必要加这个了 ~

目前在 Example 里面的代码加上(如有必要)就行 ~

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 111 to 113
def array_read(
array: list[T] | paddle.Tensor, i: paddle.Tensor
) -> T | paddle.Tensor:
Copy link
Contributor

Choose a reason for hiding this comment

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

这个地方是否还需要添加类型?

python 官方的例子是没有的:

@overload
def utf8(value: None) -> None:
    pass
@overload
def utf8(value: bytes) -> bytes:
    pass
@overload
def utf8(value: unicode) -> bytes:
    pass
def utf8(value):
    <actual implementation>

感觉多少有点冗余 ~

Copy link
Contributor

Choose a reason for hiding this comment

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

@SigureMo 帮忙看看 ~

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@gouzil gouzil changed the title [Typing][task 1] Add type annotations for paddle/tensor/array.py [Typing][A-1] Add type annotations for paddle/tensor/array.py Jun 11, 2024
SigureMo
SigureMo previously approved these changes Jun 12, 2024
Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTMeow 🐾

@SigureMo SigureMo changed the title [Typing][A-1] Add type annotations for paddle/tensor/array.py [Typing][A-1] Add type annotations for paddle/tensor/array.py Jun 12, 2024
Copy link
Contributor

@megemini megemini Jun 12, 2024

Choose a reason for hiding this comment

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

改一下 docstring 中的返回值写法吧:

    Returns:
        Tensor: 0-D Tensor with shape [], which is the length of array.

改为

    Returns:
        Tensor, 0-D Tensor with shape [], which is the length of array.

参考讨论:#65064 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@wanghuancoder wanghuancoder 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants