BUG Fixes regression for nllloss gradcheck#64203
BUG Fixes regression for nllloss gradcheck#64203thomasjpfan wants to merge 4 commits intopytorch:masterfrom
Conversation
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit e549bc7 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
| using accscalar_t = at::acc_type<scalar_t, /*is_cuda*/true>; | ||
| nll_loss_forward_reduce_cuda_kernel_2d<scalar_t, accscalar_t, index_t> |
There was a problem hiding this comment.
This was what was causing the issue with gradcheck.
jbschlosser
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the fix :)
| int n_classes, | ||
| int64_t ignore_index) { | ||
| CUDA_KERNEL_ASSERT(threadIdx.x == 0 && threadIdx.y == 0 & threadIdx.z == 0); | ||
| CUDA_KERNEL_ASSERT(threadIdx.x == 0 && threadIdx.y == 0 && threadIdx.z == 0); |
| auto weight_ = weight.defined() ? weight.contiguous() : weight; | ||
|
|
||
| if (reduction == Reduction::None & n_dims == 2) { | ||
| if (reduction == Reduction::None && n_dims == 2) { |
There was a problem hiding this comment.
here too :0 Richard says we should have a linter rule for catching this sort of thing- seems like a good idea
There was a problem hiding this comment.
The compiler does warn about these I think. Maybe we want to make these warnings errors.
There was a problem hiding this comment.
We should probably compile pytorch with -Werror for good practice. The downside is pytorch already emits a lot of warnings so fixing pytorch to get it into a good shape for this will take a while
|
@jbschlosser has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@jbschlosser has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@jbschlosser merged this pull request in a7ae73a. |
Fixes #64163
This PR includes the fix and the opinfo from #63854 for non-regression testing.
cc @albanD @mruberry @jbschlosser