Skip to content

Implementation of Weibull distribution#9454

Closed
tonyduan wants to merge 1 commit intopytorch:masterfrom
tonyduan:weibull
Closed

Implementation of Weibull distribution#9454
tonyduan wants to merge 1 commit intopytorch:masterfrom
tonyduan:weibull

Conversation

@tonyduan
Copy link
Contributor

@tonyduan tonyduan commented Jul 15, 2018 •
edited
Loading

This implements the two-parameter Weibull distribution, with scale $\lambda$ and shape $k$ parameters as described on Wikipedia.

Details

  • We implement as a transformed exponential distribution, as described here.
  • The weibull_min variance function in scipy does not yet support a vector of distributions, so our unit test uses a scalar distribution instead of a vector.

Example of the bug:

>>> sp.stats.expon(np.array([0.5, 1, 2])).var() # fine
array([1., 1., 1.])
>>> sp.stats.weibull_min(c=np.array([0.5, 1, 2])).var() # buggy
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 490, in var
    return self.dist.var(*self.args, **self.kwds)
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 1242, in var
    res = self.stats(*args, **kwds)
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 1038, in stats
    if np.isinf(mu):
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

@apaszke
Copy link
Contributor

apaszke commented Jul 15, 2018

@fritzo

vishwakftw
vishwakftw reviewed Jul 15, 2018
View reviewed changes
torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

vishwakftw
vishwakftw reviewed Jul 15, 2018
View reviewed changes
torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

vishwakftw
vishwakftw reviewed Jul 15, 2018
View reviewed changes
torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

vishwakftw
vishwakftw reviewed Jul 15, 2018
View reviewed changes
torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ssnl
Copy link
Collaborator

ssnl commented Jul 15, 2018

I'm pleasantly surprised to see a PR from you Tony. :D

-Simon

facebook-github-bot
facebook-github-bot reviewed Jul 16, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang
Copy link
Contributor

ezyang commented Jul 16, 2018

@vishwakftw Do you think this is good to go?

vishwakftw
vishwakftw approved these changes Jul 16, 2018
View reviewed changes
Copy link
Contributor

@vishwakftw vishwakftw left a comment

Choose a reason for hiding this comment

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

I can't find anything wrong with this.

cc: @fritzo @alicanb

@alicanb
Copy link
Collaborator

alicanb commented Jul 16, 2018

Yeah I took a look at this last night, looks pretty clean 👍

fritzo
fritzo reviewed Jul 16, 2018
View reviewed changes
test/test_distributions.py Outdated

This comment was marked as off-topic.

Sign in to view

fritzo
fritzo reviewed Jul 16, 2018
View reviewed changes
torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

fritzo
fritzo reviewed Jul 16, 2018
View reviewed changes
torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

fritzo
fritzo reviewed Jul 16, 2018
View reviewed changes
torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

@tonyduan
Copy link
Contributor Author

tonyduan commented Jul 16, 2018

Thanks for the comments all, especially @fritzo.

I've updated the code to take suggestions into account. Please take another look, when you find time.

facebook-github-bot
facebook-github-bot reviewed Jul 17, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

fritzo
fritzo reviewed Jul 17, 2018
View reviewed changes
torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

fritzo
fritzo reviewed Jul 17, 2018
View reviewed changes
Copy link
Collaborator

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

Looks good after a tiny bit of cleanup.

torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/distributions/weibull.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Jul 17, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ssnl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@weiyangfb
Copy link
Contributor

weiyangfb commented Jul 24, 2018 •
edited
Loading

why this PR is not merged yet? any updates? cc @ezyang @ssnl

jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 30, 2018
@tonyduan @jramseyer
Implementation of Weibull distribution (pytorch#9454)
7ce8568 
Summary:
This implements the two-parameter Weibull distribution, with scale $\lambda$ and shape $k$ parameters as described on [Wikipedia](https://en.wikipedia.org/wiki/Weibull_distribution).

**Details**
- We implement as a transformed exponential distribution, as described [here](https://en.wikipedia.org/wiki/Weibull_distribution#Related_distributions).
- The `weibull_min` variance function in scipy does not yet support a vector of distributions, so our unit test uses a scalar distribution instead of a vector.

Example of the bug:

```
>>> sp.stats.expon(np.array([0.5, 1, 2])).var() # fine
array([1., 1., 1.])
>>> sp.stats.weibull_min(c=np.array([0.5, 1, 2])).var() # buggy
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 490, in var
    return self.dist.var(*self.args, **self.kwds)
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 1242, in var
    res = self.stats(*args, **kwds)
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 1038, in stats
    if np.isinf(mu):
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()
```
Pull Request resolved: pytorch#9454

Differential Revision: D8863574

Pulled By: SsnL

fbshipit-source-id: 1ad3e175b469eee2b6af98e7b379ea170d3d9787
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@tonyduan
Implementation of Weibull distribution (pytorch#9454)
9555ebf 
Summary:
This implements the two-parameter Weibull distribution, with scale $\lambda$ and shape $k$ parameters as described on [Wikipedia](https://en.wikipedia.org/wiki/Weibull_distribution).

**Details**
- We implement as a transformed exponential distribution, as described [here](https://en.wikipedia.org/wiki/Weibull_distribution#Related_distributions).
- The `weibull_min` variance function in scipy does not yet support a vector of distributions, so our unit test uses a scalar distribution instead of a vector.

Example of the bug:

```
>>> sp.stats.expon(np.array([0.5, 1, 2])).var() # fine
array([1., 1., 1.])
>>> sp.stats.weibull_min(c=np.array([0.5, 1, 2])).var() # buggy
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 490, in var
    return self.dist.var(*self.args, **self.kwds)
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 1242, in var
    res = self.stats(*args, **kwds)
  File "/usr/local/lib/python3.7/site-packages/scipy/stats/_distn_infrastructure.py", line 1038, in stats
    if np.isinf(mu):
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()
```
Pull Request resolved: pytorch#9454

Differential Revision: D8863574

Pulled By: SsnL

fbshipit-source-id: 1ad3e175b469eee2b6af98e7b379ea170d3d9787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@fritzo fritzo fritzo approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@ssnl ssnl Awaiting requested review from ssnl

@zdevito zdevito Awaiting requested review from zdevito

@zou3519 zou3519 Awaiting requested review from zou3519

+1 more reviewer

@vishwakftw vishwakftw vishwakftw approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@tonyduan @apaszke @ssnl @ezyang @alicanb @weiyangfb @fritzo @facebook-github-bot @vishwakftw