Skip to content

Fix log_softmax and softmax if any dimension is 0-d #17651

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
bhushan23 wants to merge 1 commit into from
Closed

Fix log_softmax and softmax if any dimension is 0-d #17651

bhushan23 wants to merge 1 commit into from

Conversation

@bhushan23
Copy link
Contributor

@bhushan23 bhushan23 commented Mar 4, 2019

  • Test added
  • test_dim_function_empty: softmax and log_softmax on last dimension

fixes: #17262

ssnl
ssnl approved these changes Mar 4, 2019
View reviewed changes
Copy link
Collaborator

@ssnl ssnl left a comment

Choose a reason for hiding this comment

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

@pytorchbot merge this please

@ssnl
Copy link
Collaborator

ssnl commented Mar 4, 2019

@pytorchbot merge this please

@pytorchbot pytorchbot added the merge-this-please Was marked for merge with @pytorchbot merge this please label Mar 4, 2019
apaszke
apaszke suggested changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

Instead of modifying device-specific kernels, can't we skip those cases in top-level functions?

@ssnl ssnl removed the merge-this-please Was marked for merge with @pytorchbot merge this please label Mar 4, 2019
@ezyang
Copy link
Contributor

ezyang commented Mar 4, 2019

@bhushan23 I agree with @apaszke. Do you think you could make this change?

@bhushan23
Fix log_softmax and softmax if any dimension is 0-d
30c9e4d 
- Test added
- test_dim_function_empty: softmax and log_softmax on last dimension
@bhushan23
Copy link
Contributor Author

bhushan23 commented Mar 4, 2019 •
edited
Loading

@apaszke, done

facebook-github-bot
facebook-github-bot reviewed Mar 6, 2019
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.

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

@apaszke
Copy link
Contributor

apaszke commented Mar 10, 2019

Would be nice to add test cases for this

@bhushan23
Copy link
Contributor Author

bhushan23 commented Mar 10, 2019

Would be nice to add test cases for this

Only missing test case in forward pass was aceesing last dimension (which we have added now)
Now, since we are returning 0-d tensor in forward pass, we will have 0-d tensor during backward and autograd give Runtime error RuntimeError: element 0 of tensors does not require grad and does not have a grad_fn

Adding test case to ensure future coverage.

@bhushan23
Copy link
Contributor Author

bhushan23 commented Mar 10, 2019

@pytorchbot rebase this please

@bhushan23 bhushan23 deleted the log-softmax branch March 10, 2019 22:53
zdevito pushed a commit to zdevito/ATen that referenced this pull request Mar 10, 2019
@bhushan23 @facebook-github-bot
Fix log_softmax and softmax if any dimension is 0-d (#17651)
5704824 
Summary:
- Test added
- test_dim_function_empty: softmax and log_softmax on last dimension

fixes: #17262
Pull Request resolved: pytorch/pytorch#17651

Differential Revision: D14349009

Pulled By: gchanan

fbshipit-source-id: b6f728f5c6be8ae7615749e3f0c201886632923e
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

@gchanan gchanan gchanan approved these changes

@ssnl ssnl ssnl approved these changes

+2 more reviewers

@soumith soumith soumith approved these changes

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

CPU log_softmax on [N, 0] tensor is broken

8 participants

@bhushan23 @ssnl @ezyang @apaszke @soumith @gchanan @facebook-github-bot @pytorchbot