Skip to content

Fix bincount for empty input #9757

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
Closed
vishwakftw wants to merge 6 commits into from
Closed

Fix bincount for empty input #9757

vishwakftw wants to merge 6 commits into from

Conversation

@vishwakftw
Copy link
Contributor

@vishwakftw vishwakftw commented Jul 24, 2018 •
edited
Loading

Added tests too. Fixes #9756 .

facebook-github-bot
facebook-github-bot reviewed Jul 24, 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.

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

@zou3519
Copy link
Contributor

zou3519 commented Jul 24, 2018

I'm a little concerned because the documentation is ill-defined for what happens in this case: https://pytorch.org/docs/master/torch.html?highlight=bincount#torch.bincount . If we're trying to match numpy, we should probably make the docs clearer on what happens in this edge case.

@vishwakftw
Copy link
Contributor Author

vishwakftw commented Jul 24, 2018

Sure, I will clear up that as well.

@vishwakftw
Copy link
Contributor Author

vishwakftw commented Jul 24, 2018

@zou3519 Are we not allowed to create 0-element tensors anymore? It seems torch.tensor([], ...) is failing.

@zou3519
Copy link
Contributor

zou3519 commented Jul 24, 2018 •
edited
Loading

It's working for me though I might not be on the latest master:

In [1]: import torch

In [2]: torch.tensor([], dtype=torch.long, device='cuda')
Out[2]: tensor([], device='cuda:0', dtype=torch.int64)

I don't think we are disallowing empty tensor creation.

edit: Tested on master and empty tensor creation works, maybe the error is something else?

@vishwakftw
Copy link
Contributor Author

vishwakftw commented Jul 24, 2018

It was an nit in the ordering of the if clauses. Sorry about that.

@yf225
Copy link
Contributor

yf225 commented Jul 24, 2018

@pytorchbot retest this please

facebook-github-bot
facebook-github-bot reviewed Jul 24, 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.

@vishwakftw
Copy link
Contributor Author

vishwakftw commented Jul 30, 2018

Is this good to go?

@ailzhang
Copy link
Contributor

ailzhang commented Jul 31, 2018

@pytorchbot retest this please.

@vishwakftw
Copy link
Contributor Author

vishwakftw commented Aug 4, 2018

@ailzhang Is this good to go?

gchanan
gchanan reviewed Aug 6, 2018
View reviewed changes
torch/_torch_docs.py Outdated
:math:`out[n] += weights[i]` if :attr:`weights` is specified else
:math:`out[n] += 1`.
If the number of elements in :attr:`input` is 0, then the result is a tensor of size

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/_torch_docs.py Outdated
Shape:
output (Tensor): ``Size([max(input) + 1])``
output (Tensor): ``Size([max(input) + 1])`` if :attr:`input` is non-empty.

This comment was marked as off-topic.

Sign in to view

@ssnl
Copy link
Collaborator

ssnl commented Aug 7, 2018

@pytorchbot retest this please

@vishwakftw
Copy link
Contributor Author

vishwakftw commented Aug 7, 2018

@ssnl Is this good to go?

facebook-github-bot
facebook-github-bot reviewed Aug 7, 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.

soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Aug 10, 2018
@vishwakftw @PenghuiCheng
Fix bincount for empty input (pytorch#9757)
27d4954 
Summary:
Added tests too. Fixes pytorch#9756 .
Pull Request resolved: pytorch#9757

Differential Revision: D8966879

Pulled By: soumith

fbshipit-source-id: 9f08a9d5d5d037db16319141d7a227a5efa23869
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@vishwakftw
Fix bincount for empty input (pytorch#9757)
f3d84b3 
Summary:
Added tests too. Fixes pytorch#9756 .
Pull Request resolved: pytorch#9757

Differential Revision: D8966879

Pulled By: soumith

fbshipit-source-id: 9f08a9d5d5d037db16319141d7a227a5efa23869
@soumith soumith reopened this Aug 15, 2018
@soumith
Copy link
Contributor

soumith commented Aug 15, 2018

the merge got botched for this one. working on re-merging!

facebook-github-bot
facebook-github-bot reviewed Aug 15, 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.

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

zdevito pushed a commit to zdevito/ATen that referenced this pull request Aug 16, 2018
@vishwakftw @facebook-github-bot
Fix bincount for empty input (#9757)
0cfa29e 
Summary:
Added tests too. Fixes #9756 .
Pull Request resolved: pytorch/pytorch#9757

Reviewed By: Yangqing

Differential Revision: D9348485

Pulled By: soumith

fbshipit-source-id: e13afadf8dbea20ee6ee595383c522dcbaf8796a
@vishwakftw vishwakftw deleted the bincount-empty-fix branch August 17, 2018 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gchanan gchanan gchanan left review comments

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

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@zdevito zdevito Awaiting requested review from zdevito

+1 more reviewer

@soumith soumith soumith 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.

torch.bincount errors on empty input

9 participants

@vishwakftw @zou3519 @yf225 @ailzhang @ssnl @soumith @gchanan @facebook-github-bot @ezyang