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.

Sign up for GitHub

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

Jump to bottom

【Hackathon 4th No.11】 为 paddle 新增 paddle.distribution.Geometric 分布 #376

Merged
merged 4 commits into from
Feb 28, 2023
Merged

【Hackathon 4th No.11】 为 paddle 新增 paddle.distribution.Geometric 分布 #376

merged 4 commits into from
Feb 28, 2023

Conversation

dasenCoding
Copy link
Contributor

rfc : Geometric Distribution

@paddle-bot
Copy link

paddle-bot bot commented Feb 22, 2023

你的PR提交成功，感谢你对开源项目的贡献!
请检查PR提交格式和内容是否完备，具体请参考示例和模版。
Your PR has been submitted. Thanks for your contribution!
Please check its format and content. For this, you can refer to Template and Demo.

@dasenCoding
Copy link
Contributor Author

@luotao1 还有一个，多谢多谢！

@dasenCoding dasenCoding mentioned this pull request Feb 23, 2023
Closed
cxxly
cxxly reviewed Feb 23, 2023
View reviewed changes
rfcs/APIs/20230223_api_design_for_geometric.md Outdated

`Geometric distribution` 应用条件：进行一系列独立试验，每一次试验活成功或失败，每一次试验的成功概率相同，即：为了取得第一次成功，需要进行多少次试验。

目前 Paddle 框架中没有继承 Geometric 分布。所以此任务的目标是在 Paddle 框架中，基于现有概率分布方案，在其基础上进行扩展，新增 Geometric API，API 的调用路径为： `paddle.distribution.Geometric`。
Copy link

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.

收到！

cxxly
cxxly reviewed Feb 23, 2023
View reviewed changes
rfcs/APIs/20230223_api_design_for_geometric.md Outdated
```python
#probs: the probability of sampling 1. Must be in range (0, 1]
#logits：the log-odds of sampling 1.
paddle.distribution.geometric.Geometric(probs, logits)
Copy link

@cxxly cxxly Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

这两个参数是二选一，还是必须都传；我理解是二选一，目前Paddle只需要支持probs就行；logits如果有兴趣可以所有分布统一支持

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这两个参数是二选一，还是必须都传；我理解是二选一，目前Paddle只需要支持probs就行；logits如果有兴趣可以所有分布统一支持

  1. 确实是二选一，是我没有表达清楚。
  2. logits 的实现确实是需要额外的处理。后面能力允许的话再尝试着如何让所有分布都支持 logits。

@dasenCoding
Copy link
Contributor Author

@cxxly 您上面提出的修改意见都已进行修改，劳烦抽空再给review一下，感谢！

cxxly
cxxly approved these changes Feb 28, 2023
View reviewed changes
Copy link

@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

@luotao1 luotao1 merged commit bcafdd2 into PaddlePaddle:master Feb 28, 2023
@PaddlePaddle PaddlePaddle locked as off-topic and limited conversation to collaborators Feb 28, 2023
@PaddlePaddle PaddlePaddle deleted a comment from lwbmowgli Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cxxly cxxly cxxly approved these changes

Assignees

@luotao1 luotao1

@cxxly cxxly

@cloud2009 cloud2009

Labels
contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dasenCoding @cxxly @luotao1 @cloud2009