Accumulate loss function reduce=True into accreal instead of real #8753
Conversation
THNN was accumulating the result of reduction loss functions into real instead of accreal. This was causing precision issues with MSELoss. I added precision tests for some of the losses (KLDivLoss, BCELoss, L1Loss, SmoothL1Loss, MSELoss). The other ones are a bit harder to test.
zou3519
requested review from
apaszke,
colesbury,
ezyang,
gchanan,
soumith and
zdevito
as code owners
|
Nice! The new tests are written with good |
|
Don't merge this yet, I think some of the tests are going to fail. @ssnl yes I checked that all of precision tests that I added fail before this patch. |
|
Does it make sense to include these tests if the only reason is so that they fail before this patch and pass after it? I feel that they might not be that helpful in the future. |
|
@li-roy They can prevent regression. Haven't we always been adding tests along with fixes? |
|
@zou3519 Don't forget about this patch! |
|
I'll rebase and submit this with only the MSELoss changes. The other losses have similar precision issues but those require more looking into and aren't immediately solved by accumulating into accreal instead of real. |
|
Closing in favor of #9287 |
5 participants
THNN was accumulating the result of reduction loss functions
into real instead of accreal. This was causing precision issues with
MSELoss.
I added precision tests for some of the losses (KLDivLoss, BCELoss,L1Loss, SmoothL1Loss, MSELoss). The other ones are a bit harder
to test.
This patch only fixes MSELoss. Some of the other losses exhibit bad precision as well (because they accumulate into real instead of accreal) and require more investigation. I will open an issue for those.
Fixes #8710