Clarify difference between kl_divergence and convex_kl_divergence - #1514
Conversation
emilyfertig
left a comment
There was a problem hiding this comment.
Thank you for the PR!
This function is misnamed, since as @carlosgmartin points out on #1514 , the KL divergence is already jointly convex. A better name would be generalized_kl_divergence, as you referenced in the docstring.
If you're up for it, could you change the function name and deprecate convex_kl_divergence? This would mean adding generalized_kl_divergence to __init__.py, losses/__init__.py, and docs/api/losses.rst, deprecating convex_kl_divergence using warn_deprecated_function, and updating losses/_classification_test.py to call the new function instead.
Or, if you'd prefer, we can merge this PR since it's already an improvement, and rename the function in a follow-up.
emilyfertig
left a comment
There was a problem hiding this comment.
Thank you! This basically looks good, just a few small comments.
|
Thanks for the feedback! I've signed off for the night, but I will fix the alphabetical ordering and clean up the rebase/imports first thing tomorrow morning. |
|
I performed a clean reset to fix the bad rebase history and restored the chex-free imports. I also fixed the alphabetical ordering as requested. Ready for review! |
emilyfertig
left a comment
There was a problem hiding this comment.
Looks great, thank you!
|
Thank you for the PR! |
Description
This PR addresses issue #1513 by clarifying the documentation for
kl_divergenceandconvex_kl_divergence.The previous documentation did not explicitly state the mathematical difference between the two implementations, nor did it explain the specific use case for the convex variant.
Changes made:
kl_divergencedocstring to identify it as the standard definition.convex_kl_divergencedocstring to:sum(exp(log_predictions)) - sum(targets).Fixes #1513
Type of change