-
Notifications
You must be signed in to change notification settings - Fork 0
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
updated binsearch, such that investigated quarters #14
base: main
Are you sure you want to change the base?
Conversation
I like the idea. As we have tests for all epsilon search algorithms we should also add tests for this one in the test_epsilon_value_estimator folder |
Also, I think we should rename the module to make clear what exactly is adjusted compared to the binary search we have |
class AdjustedBinarySearchEpsilonValueEstimator(BinarySearchEpsilonValueEstimator): | ||
|
||
|
||
def get_next_epsilon(self, midpoint:int, first:int, last:int, epsilon_status_list: list[EpsilonStatus]) -> int: |
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.
Why is the epsilon status list needed here?
last = midpoint - 1 | ||
midpoint = (first + last) // 2 | ||
else: | ||
if len(epsilon_status_list)>3: |
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.
Why just in case there are more than three elements in the epsilon status list?
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.
if there are only 3, than its either last and first and midpoint, so the quartering might end up returning midpoint and then you get stuck in an endless loop. I will do this differently though
midpoint = (first + last) // 2 | ||
else: | ||
if len(epsilon_status_list)>3: | ||
midpoint = self.get_next_epsilon(epsilon_status_list) |
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 arguments do not match the function definition from above
new review required! |
from tests.test_epsilon_value_estimator.conftest import MockVerificationModule | ||
|
||
|
||
#TODO: adjust this for quartered |
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 think this todo should still be resolved
class QuarteredBinarySearchEpsilonValueEstimator(BinarySearchEpsilonValueEstimator): | ||
|
||
|
||
def get_next_epsilon(self, midpoint:int, first:int, last:int) -> int: |
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.
let's include spaces in the typehints
Did you execute the tests with the command |
Updated binary search such that if there is an error or a timeout it wont continue with the epsilon directly next to the timeout/memout epsilon, but will search further away.
We can cite Hadar Shavits work if it gets published at AAAI