-
Notifications
You must be signed in to change notification settings - Fork 253
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
[CodeStyle] 单测报错信息优化 RFC #198
[CodeStyle] 单测报错信息优化 RFC #198
Conversation
|
||
对于现有代码来说 `self.assertTrue(np.allclose(...))` 是一个非常简单的模式,其前缀 `self.assertTrue(np.allclose(` 是完全可以通过正则甚至简单的文本搜索来搜索到,但如果想要无错漏地将整个模式匹配出来进行替换,可能正则表达式并不能很好地完成(要考虑到括号是可以无限嵌套的,而正则表达式是不能表达无限嵌套的,除非将其嵌套限制在一个深度,但那样写出来的正则可读性也极差) | ||
|
||
而对于 Python 代码的解析,当然最好的方式是直接将其翻译为 Python 的语法树,然后在语法树上匹配相应的模式并进行替换即可。Python 代码到语法树的解析,我们可以利用 builtin 的 `ast` 模块,以下是一个目前实现的 `self.assertTrue(np.allclose(...))` 替换的简单 demo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
使用python语法数代替grep来做替换的想法很棒
|
||
为了避免全部肉眼判断与手动替换带来的巨大工作量,这里首先将 `self.assertTrue(np.array_equal(...))` 全部自动替换为 `np.testing.assert_array_equal(...)`,之后对现存 `np.testing.assert_array_equal(...)` 进行搜索,手动将 `float` 类型数据的替换为 `np.testing.assert_allclose(...)`。 | ||
|
||
在已有的测试中(https://github.com/PaddlePaddle/Paddle/pull/44947),已经尝试将 `self.assertTrue(np.array_equal(...))` 全部自动替换为 `np.testing.assert_array_equal(...)`,CI 没有任何问题,因此该替换是安全的,因此在不进行手工替换的情况下也是没有任何问题的。但为了让测试更加严谨,应当手动再将 `float` 的替换为 `np.testing.assert_allclose(...)`。在无法立即判断的情况下保持现状。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
和 @qili93 讨论了下:
assert_array_equal可以对float进行判断,见https://numpy.org/doc/stable/reference/generated/numpy.testing.assert_array_equal.html
因此,PaddlePaddle/Paddle#44947 在CI过了的情况下,可以先合入。
self.assertTrue(np.array_equal(...))
不管是int还是float类型,都可以用np.testing.assert_array_equal(...)
代替。- 但由于某些硬件本身的精度问题,如果使用
assert_array_equal
后CI过不了,就要用np.testing.assert_allclose(...)
,并搭配修改atol
和rtol
来过CI。
@SigureMo 可以对应修改下RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已修改,我和 @Yulv-git 再商量下分工问题后找你们 review~
9e81283
to
acb46c7
Compare
9364aeb
to
373e7b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM+++
本 RFC 对 PaddlePaddle/Paddle#44641 中提到的问题(现有单测报错信息并不友好的问题)的解决方案进行了调研与初步尝试,并进一步对具体实现方案和分工进行了明确。