{"action":"created","issue":{"url":"https://api.github.com/repos/pytorch/pytorch/issues/6786","repository_url":"https://api.github.com/repos/pytorch/pytorch","labels_url":"https://api.github.com/repos/pytorch/pytorch/issues/6786/labels{/name}","comments_url":"https://api.github.com/repos/pytorch/pytorch/issues/6786/comments","events_url":"https://api.github.com/repos/pytorch/pytorch/issues/6786/events","html_url":"https://github.com/pytorch/pytorch/pull/6786","id":316077072,"number":6786,"title":"move softmax/logsoftmax to ATen","user":{"login":"ngimel","id":15841449,"avatar_url":"https://avatars3.githubusercontent.com/u/15841449?v=4","gravatar_id":"","url":"https://api.github.com/users/ngimel","html_url":"https://github.com/ngimel","followers_url":"https://api.github.com/users/ngimel/followers","following_url":"https://api.github.com/users/ngimel/following{/other_user}","gists_url":"https://api.github.com/users/ngimel/gists{/gist_id}","starred_url":"https://api.github.com/users/ngimel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ngimel/subscriptions","organizations_url":"https://api.github.com/users/ngimel/orgs","repos_url":"https://api.github.com/users/ngimel/repos","events_url":"https://api.github.com/users/ngimel/events{/privacy}","received_events_url":"https://api.github.com/users/ngimel/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-04-19T23:40:20Z","updated_at":"2018-04-22T18:13:48Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/pytorch/pytorch/pulls/6786","html_url":"https://github.com/pytorch/pytorch/pull/6786","diff_url":"https://github.com/pytorch/pytorch/pull/6786.diff","patch_url":"https://github.com/pytorch/pytorch/pull/6786.patch"},"body":"THCUNN kernels are mostly unchanged, with minimum changes to types so that more intermediate values are preserved in AccumT. \r\nSoftmax/LogSoftmax from THNN are combined into a single templated function.\r\nRemaining issues\r\n - in THNN accumulation for float used to be in double, ATen does not support this now, would need something like https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/cuda/AccumulateType.cuh, should we add it? With this PR, accumulation for float is in float. \r\n- in softmax_backward, self argument is added just so that gradient of self can be computed in double backward. Self tensor itself is not necessary neither for softmax_backward, nor for softmax_double_backward. I don't know if there's a better way of handling it. \r\n- scalar handling is somewhat awkward (I'm checking .dim() == 0, and do .view(1) for scalars), is there a better way of doing it?\r\n- legacy tests are failing, because softmax is no longer legacy. I can rewrite legacy softmax forward to use torch.softmax instead of [backend].SoftMax_UpdateOutput, but what to do with backward? Softmax backward is no longer exposed."},"comment":{"url":"https://api.github.com/repos/pytorch/pytorch/issues/comments/383401089","html_url":"https://github.com/pytorch/pytorch/pull/6786#issuecomment-383401089","issue_url":"https://api.github.com/repos/pytorch/pytorch/issues/6786","id":383401089,"user":{"login":"ezyang","id":13564,"avatar_url":"https://avatars0.githubusercontent.com/u/13564?v=4","gravatar_id":"","url":"https://api.github.com/users/ezyang","html_url":"https://github.com/ezyang","followers_url":"https://api.github.com/users/ezyang/followers","following_url":"https://api.github.com/users/ezyang/following{/other_user}","gists_url":"https://api.github.com/users/ezyang/gists{/gist_id}","starred_url":"https://api.github.com/users/ezyang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ezyang/subscriptions","organizations_url":"https://api.github.com/users/ezyang/orgs","repos_url":"https://api.github.com/users/ezyang/repos","events_url":"https://api.github.com/users/ezyang/events{/privacy}","received_events_url":"https://api.github.com/users/ezyang/received_events","type":"User","site_admin":false},"created_at":"2018-04-22T18:13:48Z","updated_at":"2018-04-22T18:13:48Z","author_association":"CONTRIBUTOR","body":"> in THNN accumulation for float used to be in double, ATen does not support this now, would need something like https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/cuda/AccumulateType.cuh, should we add it? With this PR, accumulation for float is in float.\r\n\r\nYes, that would be great. I'd probably make another version of this template for \"AccAccumulateType\" or similar (following the TH convention)\r\n\r\n> in softmax_backward, self argument is added just so that gradient of self can be computed in double backward. Self tensor itself is not necessary neither for softmax_backward, nor for softmax_double_backward.\r\n\r\nI'm a little perplexed by the double-backwards situation; if `_backward` has no data dependence on `self`, then there shouldn't be any gradient formula for the double backwards, right?\r\n\r\nCC @gchanan about scalars"}}
{"actor":{"display_login":"ezyang"}}