-
Notifications
You must be signed in to change notification settings - Fork 173
Fix Evidence threshold bug #185
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
Fix Evidence threshold bug #185
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.
The change looks good! Will review properly when it's a full PR but don't see any issue with the updated code.
), "Percentage must be between 0 and 100" | ||
max_global_evidence = self.current_mlh["evidence"] | ||
x_percent_of_max = max_global_evidence * (percentage / 100) | ||
return max_global_evidence - x_percent_of_max | ||
elif self.evidence_update_threshold == "x_percent_threshold": |
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.
suggestion (non-blocking): Perhaps rename "x_percent_threshold"
and the variable to something else? Now that evidence_update_threshold
can take an "x percent" threshold value (e.g., "80%"), this name becomes quite confusing to parse.
note: I'm generally confused what x
means.
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.
I was planning to do this in a separate PR (as it would touch a lot of the other code). Is that ok?
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.
Sorry, I should have mentioned that it was a non-blocking suggestion. I have no issues with that being a separate PR, just wanted to highlight my confusion.
Just turned this into a real PR as it doesn’t affect the benchmark results. It just fixes the bug. Then we can do a follow-up PR with updating to optimal parameters and updating the benchmark results with those. |
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.
Looks great, thanks for fixing this so quickly!
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.
There was a bug in the
_get_evidence_update_threshold
function in the EvidenceLM where instead of setting the threshold for upding a hypothesis tox_percent_threshold
it was set to1 - x_percent_threshold
.Results are as expected:
Given that the accuracy decreases so much after the fix, it seems like it would be good to create a separate variable for the evidence update threshold (if it is a percentage) instead of using the x_percent_threshold variable defined for the terminal condition. This PR includes a proposed way to add an option to specify
evidence_update_threshold
as a percentage.I ran some hyperparameter tests for different values:

Remaining tasks before this can become a full PR:
x_percent_threshold
value, given that these two parameters as disentangled now