Add new reduction mode in kl_div #14457
Conversation
|
@ssnl I added a test to compare it with |
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
ailzhang
changed the title
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
torch/nn/functional.py
Outdated
| specifying either of those two args will override :attr:`reduction`. Default: 'mean' | ||
| 'none' | 'batchmean' | 'sum' | 'mean'. 'none': no reduction will be applied, | ||
| 'batchmean': the sum of the output will be divided by the number of | ||
| batches in the output, 'sum': the output will be summed, 'mean': the output will be |
There was a problem hiding this comment.
"the number of batches" -> "the batch size" or "the number of elements in the input batch"
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pytorchbot retest this please |
aten/src/ATen/core/Reduction.h
Outdated
| None, // Do not reduce | ||
| Mean, // (Possibly weighted) mean of losses | ||
| Sum, // Sum losses | ||
| BatchMean, // Mean over batches. = Sum / batchsize |
There was a problem hiding this comment.
Can't you just hack this up in python? It should only be a workaround for kl_div and doesn't make sense for other losses.
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
torch/nn/functional.py
Outdated
| reduction_enum = _Reduction.legacy_get_enum(size_average, reduce) | ||
| else: | ||
| if reduction == 'mean': | ||
| warnings.warn("reduction=mean doesn't give the true kl divergence value. " |
There was a problem hiding this comment.
Better to put quotes around mean and batchmean. Also, I think this can be worded more clearly like
reduction: "mean" divides the total loss by both the batch size and the support size.
"batchmean" divides only by the batch size, and aligns with the KL divergence math definition.
"mean" will be changed to behave the same as "batchmean" in the next major release.
torch/nn/functional.py
Outdated
| 'mean': the output will be divided by the number of elements in the output | ||
| Note: :attr:`size_average` and :attr:`reduce` are in the process of being deprecated, | ||
| and in the meantime, specifying either of those two args will override :attr:`reduction`. | ||
| Note: `reduction='mean'` doesn't return the true kl divergence value, please use |
There was a problem hiding this comment.
I would make this a more obvious note using .. note::. Same for the module doc.
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
6 participants
Fixes #6622 .
We used to average over all elements for kl divergence, which is not aligned with its math definition.
This PR corrects the default reduction behavior of KL divergence that it now naverages over batch dimension.
reduction=meanaverages over batch dimension. While for most other loss functions,reduction=meanaverages over all elements.batchmeanwhich has the correct behavior for KL. Add a warning to makebatchmeanas default for KL instead ofmeanin next major release.