-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Multinomial raise error #12490
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
Multinomial raise error #12490
Conversation
| 2, | ||
| "invalid multinomial distribution (sum of probabilities <= 0)"); | ||
| THArgCheckWithCleanup((n_categories - n_zeros >= n_sample), | ||
| THCleanup(THDoubleTensor_free(cum_dist); if (start_dim == 1) THTensor_(squeeze1d)(prob_dist, prob_dist, 0);), |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
facebook-github-bot
left a comment
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.
ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
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.
ailzhang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…inomial_raise_error
facebook-github-bot
left a comment
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.
ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Fixes #12260 #2896 ``` torch.multinomial(torch.FloatTensor([0, 1, 0, 0]), 3, replacement=False) ``` The old behavior is that we return `0` after we run out of postive categories. Now we raise an error based on discussion in the issue thread. - Add testcase for cpu & cuda case, in cuda case `n_samples=1` is a simple special case, so we test against `n_sample=2` instead. Pull Request resolved: pytorch/pytorch#12490 Differential Revision: D10278794 Pulled By: ailzhang fbshipit-source-id: d04de7a60f60d0c0d648b975db3f3961fcf42db1
|
The documentation at https://pytorch.org/docs/master/torch.html#torch.multinomial does not reflect the change in behavior. The example should be updated to reflect versions 1.x. |
Summary: Update documentation to raise awareness of the fix in pytorch#12490. Thanks matteorr for pointing this out! Pull Request resolved: pytorch#17269 Reviewed By: ezyang Differential Revision: D14138421 Pulled By: ailzhang fbshipit-source-id: 6433f9807a6ba1d871eba8e9d37aa6b78fa1e1fd
|
I think it makes sens to print a meaningful error message. I just get
Code to reproduce: |
|
@asanakoy due to limitations with CUDA, we cannot print a better error message |
Fixes #12260 #2896
The old behavior is that we return
0after we run out of postive categories. Now we raise an error based on discussion in the issue thread.n_samples=1is a simple special case, so we test againstn_sample=2instead.