Skip to content

[CodeStyle][F631] Remove parentheses in assertion - #46714

Closed
caolonghao wants to merge 4 commits into
PaddlePaddle:developfrom
caolonghao:F631/fix
Closed

caolonghao wants to merge 4 commits into
PaddlePaddle:developfrom
caolonghao:F631/fix

Conversation

@caolonghao

@caolonghao caolonghao commented Oct 2, 2022 •

Copy link
Copy Markdown
Contributor

PR types

Others

PR changes

Others

Describe

Remove parentheses in assertion so that it won't always be true

Related Links

Flake8 tracking issue: #46039
配置文件更新: #46715
Fix: cattidea/paddle-flake8-project#58

@paddle-bot

paddle-bot Bot commented Oct 2, 2022

Copy link
Copy Markdown

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

assert len(to_fp16_var_names) > 0
for var_name in to_fp16_var_names:
assert (block.var(var_name).dtype, paddle.float16)
assert block.var(var_name).dtype, paddle.float16

@SigureMo SigureMo Oct 4, 2022 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

assert 语句第一个位置为判断是否为 truthy value,第二个位置是 assert 失败时的报错信息,将 paddle.float16 作为报错信息应该是不对的

根据 87 行的使用方式(self.dtype_check(self.main_prog, to_fp16_var_names))及上下文,猜测这里应该是检测所有 var.dtype 均为 paddle.float16,所以这里是否应该改成 assert block.var(var_name).dtype == paddle.float16?@gglin001

如果是判等用 self.assertEqual(block.var(var_name).dtype, paddle.float16) 也许会更好些,当然最好加上报错信息,比如 "Expected dtype of {} to be {}, but got {}".format(var_name, paddle.float16, block.var(var_name).dtype)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

有道理,我一开始对 assert 的使用理解有误。使用 assertEqual() 确实更明确。

@gglin001 gglin001 Oct 4, 2022 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

感谢发现了这个问题!

  1. 是的, 这里使用 self.assertEqual() 更合适, 之前的写法是有问题的
  2. 这里的 fp16 dtype 判断是不合适的, to_fp16_var_names 未必 全是 fp16(会有混合精度的情况), 应当去除 self.dtype_check() 这个函数

我在 新的pr里面 #46739 做了修改

@SigureMo SigureMo Oct 4, 2022 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@caolonghao 既然 #46739 已经有了更加准确的修复,本 PR 就先 close 吧~配置文件的 PR 可以保留,并在说明里链接上本 Comment 和 #46739 即可,辛苦啦~

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

收到~

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@paddle-bot

paddle-bot Bot commented Oct 4, 2022

Copy link
Copy Markdown

很抱歉,经过我们的反复讨论,你的PR暂未达到合入标准,请阅读飞桨原生算子开发规范,你可以重新提交新的PR,我们先将此PR关闭,感谢你的贡献。
Sorry to inform you that through our discussion, your PR fails to meet the merging standard (Reference: Paddle Custom Operator Design Doc). You can also submit an new one. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor External developers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[F631] assertion test is a tuple, which is always True

4 participants