fix: clamp KL divergence to non-negative values#231
fix: clamp KL divergence to non-negative values#231JasonOA888 wants to merge 3 commits intop-e-w:masterfrom
Conversation
- KL divergence is nonnegative by definition - Floating point arithmetic can produce -0.0000 - Clamp to 0.0 to avoid displaying negative zero Fixes p-e-w#141
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the Kullback-Leibler (KL) divergence calculation could yield negative values, which is mathematically incorrect. By introducing a clamping operation, the system now guarantees that the KL divergence metric remains non-negative, thereby enhancing the accuracy and reliability of the evaluation process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly clamps the KL divergence value to be non-negative, which is a valid fix since KL divergence cannot be negative by definition. I have one minor suggestion to align a new comment with the repository's style guide.
src/heretic/evaluator.py
Outdated
| reduction="batchmean", | ||
| log_target=True, | ||
| ).item() | ||
| # Clamp to non-negative (KL divergence cannot be negative by definition) |
There was a problem hiding this comment.
This comment should end with a period to adhere to the repository's style guide.
| # Clamp to non-negative (KL divergence cannot be negative by definition) | |
| # Clamp to non-negative (KL divergence cannot be negative by definition). |
References
- Comments should start with a capital letter and end with a period. They should use correct grammar and spelling. (link)
|
This should no longer be an issue with #194. Are you still seeing negative KLDs for some models? |
|
Thanks for the pointer! I checked PR #194 - it fixes KL divergence measurement issues caused by prefix detection, but Issue #141 is specifically about floating-point Even with #194 merged, the computation can still produce negative zero due to floating-point arithmetic (not a logical bug). The fix here simply clamps the value to non-negative before formatting. I don't have a specific model to reproduce this right now - I created the fix based on the issue description. If this isn't occurring in practice anymore, feel free to close both the issue and this PR. |
|
Yeah, I think we should actually keep the current behavior, since this should never occur in practice, and if it does (as it has in the past), it's indicative of a bug, and it's best if the value sticks out like a sore thumb in that case. Note also that the KLD logic is currently undergoing a major revision in #53. Thanks anyway for the PR! |
2 participants
Fixes #141