-
-
Notifications
You must be signed in to change notification settings - Fork 239
Feature/adaptive capping #1247
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
base: development
Are you sure you want to change the base?
Feature/adaptive capping #1247
Conversation
… max_runtime during init of an intensifer
…ly, added some more basic explanations to elaborate on the basic functionality and inner workings of adaptive capping.
… successfully applied.
…eature/adaptive_capping
…ing works. So far adaptive capping itself is not tested but the intensification scheme making a bug in the intensification scheme visible.
Fixed mypy issues 👍 |
return runtime | ||
except TimeoutException as e: | ||
print(f"Timeout for configuration {config} with runtime budget {budget}") | ||
return budget # FIXME: what should be returned here? |
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.
is the fixme comment still relevant?
# We want to run five random configurations before starting the optimization. | ||
initial_design = HyperparameterOptimizationFacade.get_initial_design(scenario, n_configs=5) | ||
|
||
intensifier = Intensifier(scenario, runtime_cutoff=10, adaptive_capping_slackfactor=1.2) |
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.
is AC automatically turned on?
multi_objective_algorithm: AbstractMultiObjectiveAlgorithm | None = None, | ||
runhistory_encoder: AbstractRunHistoryEncoder | None = None, | ||
config_selector: ConfigSelector | None = None, | ||
runtime_cutoff: int | None = None, |
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 does this need to be specified in the facade?
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.
and if it needs to be specified here, the docstring would be missing (weird that pre-commit does not catch that)
def uses_budgets(self) -> bool: | ||
"""If the intensifier needs to make use of budgets.""" | ||
raise NotImplementedError | ||
return False |
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 default set to False?
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.
bc it is an abstract method
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 put it that way because otherwise tests would fail due to the NotImplementedError. However since it is an abstractmethod we should probably do even nothing here? i.e., just stating "pass"? But I am not sure because this is not a normal function but a property
raise UnsupportedError() | ||
|
||
# compute the already used runtime for the challenger across instances | ||
# FIXME: in the case of multiple seeds per instance, we need to imagine |
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.
is this still relevant?
|
||
# Let's mark the first trial as finished | ||
# The config should become an incumbent now. | ||
runhistory.add(config=trial.config, cost=10, time=0.0, instance=trial.instance, seed=trial.seed, force_update=True) |
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 this formatted weirdly?:D
self.log_counter = 0 | ||
|
||
# log list and map for easier access of performance data | ||
self.log_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.
I don't really understand what is tested here, could you add more comments and/or the general idea?
Re-added adaptive capping to SMAC so that it can be used for algorithm configuration problems that allow to preemptively cap the execution of a configuration on an instance.