-
Notifications
You must be signed in to change notification settings - Fork 22.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make kl_div accept target in log space #34586
Conversation
💊 CircleCI build failures summary and remediationsAs of commit cf4814f (more details on the Dr. CI page):
XLA failureJob pytorch_xla_linux_xenial_py3_6_clang7_build is failing. Please create an issue with title prefixed by 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 on the GitHub issue tracker. This comment has been revised 129 times. |
32acc63
to
e6063ce
Compare
Needs some benchmarks for the new codepath. Compare |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests, benchmarks
If I sample from Dirichlet distributions of different size and then measure the performance, would that be sufficient? |
SGTM |
62c3209
to
aebcd59
Compare
OK, tests are fixed. TODO benchmarks.. |
d667941
to
97e195c
Compare
d33608a
to
cf4814f
Compare
Hopefully fixed now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@@ -294,7 +294,8 @@ class KLDivLoss(_Loss): | |||
|
|||
As with :class:`~torch.nn.NLLLoss`, the `input` given is expected to contain | |||
*log-probabilities* and is not restricted to a 2D Tensor. | |||
The targets are given as *probabilities* (i.e. without taking the logarithm). | |||
The targets are given as *probabilities* by default, but could be passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this isn't really accurate (and I realize this was before your change) -- the targets are given however the users give them. They are interpreted as probabilities by default.
Summary: Fixes doc for KLDivLoss as per [this comment](#34586 (comment)). Pull Request resolved: #36137 Differential Revision: D20932395 Pulled By: gchanan fbshipit-source-id: ecc395e6bc689fbf758e2cdca946049de8963856
Summary: Fixes doc for KLDivLoss as per [this comment](pytorch#34586 (comment)). Pull Request resolved: pytorch#36137 Differential Revision: D20932395 Pulled By: gchanan fbshipit-source-id: ecc395e6bc689fbf758e2cdca946049de8963856
Fixes 32520, implements 34536.
Here are some benchmarks:
Output: