-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
【Hackathon No.60】refactor unary sparse ops and add sparse sqrt, tanh, sin #41356
Changes from 1 commit
97ac270
26f4662
a7f3410
d4310af
7e5f102
71864fd
a99a5ba
95aa0b3
f706dea
d898df7
b770f41
f92e8cd
394ce5e
c577f46
3ad6fba
56fc5da
1f18c59
c606825
5dd4507
f59fa26
dea61c7
60c7359
ad8ceda
178dd27
1ace46f
7bb41d7
790cb0d
c44ac74
d35e923
b04ab6c
fa93d7d
a6d2cd0
67d14b4
268ac34
39c9750
06787c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,5 +16,21 @@ limitations under the License. */ | |
|
||
#include "paddle/phi/kernels/sparse/utils.h" | ||
|
||
DEFINE_AND_REGISTER_SPARSE_UNARY_KERNEL(sparse_relu, ReluKernel) | ||
DEFINE_AND_REGISTER_SPARSE_UNARY_KERNEL(sparse_sqrt, SqrtKernel) | ||
DEFINE_AND_REGISTER_SPARSE_UNARY_KERNEL(relu, ReluKernel) | ||
DEFINE_AND_REGISTER_SPARSE_UNARY_KERNEL(sqrt, SqrtKernel) | ||
|
||
namespace phi { | ||
namespace sparse { | ||
|
||
template <typename T, typename Context> | ||
void SparseCooXX(const Context& dev_ctx, | ||
const SparseCooTensor& x, | ||
SparseCooTensor* out) { | ||
} | ||
} // namespace sparse | ||
} // namespace phi | ||
|
||
PD_REGISTER_KERNEL( | ||
sparse_coo_xx, CPU, ALL_LAYOUT, phi::sparse::SparseCooXX, float, double) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 【需要帮助】 我这里观察到这样的现象:只有 .cc 文件里显式出现 PD_REGISTER_KERNEL 这个字符串,本文件对 kernel 的注册才会真正生效,导致我没办法正常使用我封装的 DEFINE_AND_REGISTER_SPARSE_UNARY_KERNEL 这个宏。目前我通过显式调用 PD_REGISTER_KERNEL 注册一个空的 kernel 来让本文件生效,但这显然不是一个好办法。 另一方面,用字符串匹配而不是语法解析来处理源码,我认为是比较外行的行为,会非常容易遇到意料之外的问题,进而导致只有了解内情的内部人员才知道该怎么写代码,大大加大贡献者提交代码的门槛(例如我这次遇到的问题就是一个例子),不知 paddle 团队是如何考虑的 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 您的考虑也是合理的,但我们这样做的缘由根本上也是为了减少开发者需要关注的信息:
这样的问题在各个框架中都存在,其他竞品也有手写声明的,paddle从早期版本到现在一直是采用这样的方式避免开发者额外关注声明语句的。 另外,paddle是一个很复杂的系统,规范和标准化也是很重要的,除了降低开发理解成本的考虑,我们也希望kernel的注册形式是标准化的,统一的,多少会降低一些灵活性,但这样长期演变下去,整体上代码更便于维护和理解,我们是希望限制开发者封装多样的自定义宏这种行为的。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tiancaishaonvjituizi 所以,这里建议还是把kernel的注册代码写到对应的 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chenwhql 谢谢回答,有一些疑问:
有考虑过使用 --whole-archive 吗,如果担心让库大小变大,可以把所有 kernel 的实现单独作为一个库,只对这个库开 --whole-archive。此外,
匹配和解析有很多种办法,目前这种在代码文本中匹配固定字符串的做法是很奇怪、很脆弱的,通用的做法是用 libclang 解析 AST。不过我认为最合理的做法是要求 kernel 开发者将 kernel 的信息写在一个 yaml 文件里,用代码生成来生成相关的 boilerplate code
坦率的讲我认为这个是站不住脚的,这样长期演变下去,代码并不会更便于维护,软件工程里有 “Don't Repeat Yourself” 原则,它讲述的就是如果同样的逻辑重复在多个地方,就会增加后续修改的成本并增加修改后出错的可能性。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
好的,这次我会先这样做 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tiancaishaonvjituizi 非常感谢建议哈,目前是这样
|
||
kernel->InputAt(0).SetDataLayout(phi::DataLayout::SPARSE_COO); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,18 +44,17 @@ def test_sparse_coo_sqrt(self): | |
sparse_act_out = _C_ops.final_state_sparse_coo_sqrt(sparse_coo_x) | ||
tiancaishaonvjituizi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
correct_result = [2, np.sqrt(2), 4] | ||
actual_result = sparse_act_out.non_zero_elements().numpy() | ||
assert np.array_equal(correct_result, actual_result) | ||
assert np.allclose(correct_result, actual_result) | ||
|
||
def test_sparse_csr_sqrt(self): | ||
with _test_eager_guard(): | ||
x = [[0, 4, 0, 2], [0, 0, 0, 0], [0, 0, 16, 0]] | ||
dense_x = paddle.to_tensor(x, dtype='float32') | ||
sparse_dim = 2 | ||
sparse_coo_x = dense_x.to_sparse_csr(sparse_dim) | ||
sparse_coo_x = dense_x.to_sparse_csr() | ||
sparse_act_out = _C_ops.final_state_sparse_csr_sqrt(sparse_coo_x) | ||
correct_result = [2, np.sqrt(2), 4] | ||
actual_result = sparse_act_out.non_zero_elements().numpy() | ||
assert np.array_equal(correct_result, actual_result) | ||
assert np.allclose(correct_result, actual_result) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 测试已通过 |
||
|
||
|
||
if __name__ == "__main__": | ||
|
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.
这里 paddle 原本的代码是错误的,不知道是不是没有测试过。SparseCsrTensor 的构造函数内会检查 dims 的长度,只允许 2d 和 3d,这里 1d 的 dim 会导致 check 失败
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.
当前文件数超了最大限制了,要不把api_gen_utils.cc和sparse_csr_tensor.cc这两个修复的文件单独提一个PR?
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.
好的~