-
Notifications
You must be signed in to change notification settings - Fork 10
Fixing entropy calculation #85
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
Conversation
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.
Since fixing a bug in a narrow, fairly self contained piece of the codebase, lets add unit tests while we're here?
+1 to this |
510e13a
to
2ae4962
Compare
Okay... modified the code to calculate both token and sequence entropies and added unit tests for both. |
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.
LGTM! Thanks for the fix and tests
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.
mostly lg, some nits!
76afd34
to
8a21bfc
Compare
I made a mistake when logging entropy by only looking at the actions rather than the entire distribution.
This fixes that mistake and now does correct entropy calculation
mlflow of old and new entropy logging
Update
I've modified the PR to include more functionalities: