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

[phi]migrate increment addmm multinomial cholesky kernels to phi #39858

Merged
merged 5 commits into from
Feb 24, 2022

Conversation

Aganlengzi
Copy link
Contributor

@Aganlengzi Aganlengzi commented Feb 23, 2022

PR types

Others

PR changes

Others

Describe

Migrate increment addmm multinomial and cholesky kernel to phi.

TODO: migrate InferShape funcs. move MultinomialFunctor to paddle/phi/kernels/funcs.

const DenseTensor& x,
int num_samples,
bool replacement,
DenseTensor* out);

template <typename T>
void MultinomialFunctor(int64_t* out_data, const T* in_data,
Copy link
Contributor

Choose a reason for hiding this comment

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

MultinomialFunctor 不要放到kernel 声明的头文件中,可以放到paddle/phi/kernels/funcs 下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

多谢,我在迁InferShape函数的PR中修改.

Copy link
Contributor

Choose a reason for hiding this comment

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

可以下个PR 再移动

Copy link
Contributor

@MingMingShangTian MingMingShangTian left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@chenwhql chenwhql left a comment

Choose a reason for hiding this comment

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

LGTM for overall,细节后续PR可以完善下

@@ -0,0 +1,28 @@
/* Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2022?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

多谢


namespace phi {

KernelSignature AddmmOpArgumentMapping(const ArgumentMappingContext& ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这个和opmaker的顺序一致,后面PR应该可以删除

Copy link
Contributor Author

Choose a reason for hiding this comment

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

了解,InferShape迁移中会移除。


namespace phi {

KernelSignature CholeskyOpArgumentMapping(const ArgumentMappingContext& ctx) {
Copy link
Contributor

@chenwhql chenwhql Feb 24, 2022

Choose a reason for hiding this comment

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

同上,应该也不需要写,默认生成的就可以用,这些ArgumentMapping函数之所以放到compat目录下,就是希望将来可以移除的,所以我们不写不必要的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

了解,InferShape迁移中会移除。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants