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

【Hackathon 5th No.18】Add Binomial kernel for Hackthon No. 18 -part #59690

Merged
merged 13 commits into from
Dec 13, 2023

Conversation

NKNaN
Copy link
Contributor

@NKNaN NKNaN commented Dec 5, 2023

PR types

Others

PR changes

OPs

Description

Add Binomial kernel for Hackthon No. 18: pr #57856

Copy link

paddle-bot bot commented Dec 5, 2023

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

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 7, 2023

这个test总是超时,请问可以上调一下test的时间吗

@luotao1
Copy link
Contributor

luotao1 commented Dec 7, 2023

可以的,你先上调下test时间看下。这个test大概需要多少时间?

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 7, 2023

PR-CI-Coverage里面看是15点多秒,超了一点

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 7, 2023

是在 CMakeLists.txt 里加时间吗,不太会这个,有没有参考

@luotao1
Copy link
Contributor

luotao1 commented Dec 7, 2023

image 对的,你可以搜一下`PROPERTIES TIMEOUT`这样的写法

@luotao1
Copy link
Contributor

luotao1 commented Dec 8, 2023

PR-CI-Coverage里面看是15点多秒,超了一点

那TIMEOUT放到30估计就够了,120太多了。可以等 @cxxly 其他意见一起修改

2023-12-08 06:06:47     Start 1352: test_binomial_op
2023-12-08 06:07:09     Test #1352: test_binomial_op .................   Passed   21.96 sec
2023-12-08 06:07:09     Start 1352: test_binomial_op
2023-12-08 06:07:29     Test #1352: test_binomial_op .................   Passed   20.11 sec
2023-12-08 06:07:29     Start 1352: test_binomial_op
2023-12-08 06:07:49 1/1 Test #1352: test_binomial_op .................   Passed   20.32 sec

@luotao1 luotao1 changed the title 【Hackathon 5th No.18】Add Binomial kernel for Hackthon No. 18 【Hackathon 5th No.18】Add Binomial kernel for Hackthon No. 18 -part Dec 8, 2023
@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 8, 2023

那TIMEOUT放到30估计就够了,120太多了。可以等 @cxxly 其他意见一起修改

好的,谢谢

@@ -102,6 +102,74 @@ def bernoulli(x, name=None):
return out


def binomial(total_count, prob, name=None):
Copy link
Contributor

@cxxly cxxly Dec 8, 2023

Choose a reason for hiding this comment

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

同上,签名和torch保持一致

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

@@ -323,6 +323,14 @@
func: bincount
optional: weights

- op : binomial
args : (Tensor total_count, Tensor prob)
Copy link
Contributor

@cxxly cxxly Dec 8, 2023

Choose a reason for hiding this comment

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

签名和torch一致,看了下第一个参数应该是count

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

const MetaTensor& prob,
MetaTensor* out,
MetaConfig config = MetaConfig());

Copy link
Contributor

@cxxly cxxly Dec 8, 2023

Choose a reason for hiding this comment

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

同上,签名统一修改下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改


def verify_output(self, outs):
hist, prob = output_hist(np.array(outs[0]), self.n, self.p, a=5, b=15)
np.testing.assert_allclose(hist, prob, rtol=0, atol=0.01)
Copy link
Contributor

Choose a reason for hiding this comment

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

注明阈值设置参考标准

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已注释,参照 test_bernoulli_op,test_poisson_op 和 test_multinomial_op

if not paddle.is_compiled_with_cuda():
return

print("Test Fixed Random number on GPU------>")
Copy link
Contributor

Choose a reason for hiding this comment

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

删除print

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已删除

@cxxly
Copy link
Contributor

cxxly commented Dec 8, 2023

binomial设计与实现麻烦也补充到rfc,rfc与代码实现需要保持一致

@@ -0,0 +1,37 @@
// Copyright (c) 2022 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 -> 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

@@ -0,0 +1,42 @@
// Copyright (c) 2022 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.

同上

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

PD_REGISTER_KERNEL(
binomial, CPU, ALL_LAYOUT, phi::BinomialKernel, float, double) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

输出固定为int64类型需要做类似如下的data type设置:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改,参照 multinomial 的

@@ -0,0 +1,134 @@
/* Copyright (c) 2022 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 -> 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

@@ -0,0 +1,199 @@
/* Copyright (c) 2022 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 -> 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

@@ -0,0 +1,272 @@
# Copyright (c) 2018 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.

2018 -> 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 8, 2023

binomial设计与实现麻烦也补充到rfc,rfc与代码实现需要保持一致

好的,谢谢

Copy link
Contributor

@cxxly cxxly 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

@jeff41404 jeff41404 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

@sunzhongkai588 sunzhongkai588 left a comment

Choose a reason for hiding this comment

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

LGTM,just 有一点疑问

>>> p = paddle.to_tensor([0.2, 0.6])
>>> out = paddle.binomial(n, p)
>>> print(out)
>>> # doctest: +SKIP("Random output")
Copy link
Contributor

Choose a reason for hiding this comment

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

加 seed 也不能固定输出吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

啊,这里应该是固定的,文档抄过来没去掉 SKIP
需要去掉一下吗

Copy link
Contributor

Choose a reason for hiding this comment

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

@NKNaN 你再提一个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.

好的

@luotao1 luotao1 merged commit 7b81bd8 into PaddlePaddle:develop Dec 13, 2023
28 of 29 checks passed
@NKNaN NKNaN deleted the hack18-kernel branch February 2, 2024 04:08
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.

6 participants