Fix poisson_nll_loss with full option #28637
Conversation
test/common_nn.py
Outdated
| target_fn=lambda: torch.randn(2, 3, 4, 5).floor_().abs_(), | ||
| reference_fn=lambda i, t, _: (i.exp() - t.mul(i)).mean(), | ||
| desc='no_full_loss', | ||
| check_forward_only=False, |
There was a problem hiding this comment.
I found that criterion_tests doesn't test backward by default. So, I added check_forward_only=False explicitly here for easier code review.
But, I suppose, such default is intended for faster test execution time, so I'll remove this if it's desired so.
There was a problem hiding this comment.
@mruberry do you know what is our take on this? Should we set this back to the default before merging?
There was a problem hiding this comment.
It looks like PyTorch tests backwards in module_tests and not criterion_tests (per @hi-ogawa's comment). Without a compelling reason I wouldn't change this behavior. NLLLoss is also tested by the module tests. (Unfortunately our test suite is a confusing mess of redundancy.)
There was a problem hiding this comment.
I guess the point here is that since the function itself is autodiffed, there is no point to check the gradient computations.
@hi-ogawa could you set this flag back to the default value (remove the line).
There was a problem hiding this comment.
@mruberry @albanD Thanks a lot for the review. Right, poissonnllloss_no_reduce_test (Line 280) is included in new_module_tests instead of new_criterion_tests and that one actually checks backward. Okay, I removed check_forward_only=False to align with other tests here and I hope it's ready to merge now.
test/common_nn.py
Outdated
| target_fn=lambda: torch.randn(2, 3, 4, 5).floor_().abs_(), | ||
| reference_fn=lambda i, t, _: (i.exp() - t.mul(i)).mean(), | ||
| desc='no_full_loss', | ||
| check_forward_only=False, |
There was a problem hiding this comment.
@mruberry do you know what is our take on this? Should we set this back to the default before merging?
|
@hi-ogawa The python linter complains that you should put 2 spaces before the inline comments in the test file. Could you add that quickly please? |
|
@albanD thanks a lot for the comment. I just fixed lint failures and flake CI looks okay now. |
|
Looks good. Thanks for the quick update. |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Thanks for updating. |
Summary: This fixes pytorch/pytorch#28575. It seems `poisson_nll_loss` was implemented with the incorrect assumption about `masked_select`, which actually doesn't return tensor with the same storage, so in-place operation used there didn't work as intended. Here I used `masked_fill` instead. Also, the existing test didn't have `reference_fn`, so I added it (although it's not fundamentally useful since current cpp `poisson_nll_loss` itself does exactly same algorithm as `reference_fn`). Thanks in advance for reviewing this PR. Pull Request resolved: pytorch/pytorch#28637 Differential Revision: D18299724 Pulled By: albanD fbshipit-source-id: 1aac5b20e77bf54874b79018207ba8f743766232
5 participants
This fixes #28575.
It seems
poisson_nll_losswas implemented with the incorrect assumption aboutmasked_select, which actually doesn't return tensor with the same storage, so in-place operation used there didn't work as intended.Here I used
masked_fillinstead.Also, the existing test didn't have
reference_fn, so I added it (although it's not fundamentally useful since current cpppoisson_nll_lossitself does exactly same algorithm asreference_fn).Thanks in advance for reviewing this PR.