-
Notifications
You must be signed in to change notification settings - Fork 45
bayesian-optimizer #602
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: main
Are you sure you want to change the base?
bayesian-optimizer #602
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (79.04%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
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.
Hi @spline2hg, thanks for the great PR.
The PR is quite good already but I have made many small comments about the code style.
I'm happy to talk if there are questions.
|
||
- **verbose** (int): Verbosity level from 0 (silent) to 3 (most verbose). Default is 2. | ||
|
||
- **kappa** (float): Parameter to balance exploration vs exploitation in UCB. Higher values |
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.
We need to mention more explicitly that this parameter is only used if the ucb acquisition function is used and the user did not pass a configured instance of an AcquisitionFunction object. Same for xi below.
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.
Also, we should not use all UPPERCASE notation for UCB and not use the abbreviation. Documentation can be verbose as long as it is precise. I.e. it would be better to write: "Paremeter to balance the exploration vs. exploitation trade-off if the upper_confidence_bound
acquisition function is used."
- "ucb" or "upper_confidence_bound": Upper Confidence Bound | ||
- "ei" or "expected_improvement": Expected Improvement | ||
- "poi" or "probability_of_improvement": Probability of Improvement | ||
Default is None (uses package default). |
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.
What is the package default?
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.
UpperConfidenceBound if no constraints are passed; otherwise ExpectedImprovement.
- **init_points** (int): Number of random exploration points to evaluate before | ||
starting optimization. Default is 5. | ||
|
||
- **n_iter** (int): Number of Bayesian optimization iterations to perform after | ||
the initial random exploration. Default is 25. |
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.
How were these numbers chosen? Are they just the defaults from the package? I would have expected that those should rather be adaptive (e.g. getting larger for higher dimensional problems) but it is ok to go with the package defaults. Just needs to be documented where the numbers come from.
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.
Yes, these are the package defaults. Should we explicitly mention in the documentation that these values come from the package defaults? Also, to make them adaptive, how could we relate these parameters to the dimensionality of the problem?
- **xi** (float): Parameter to balance exploration vs exploitation in EI and POI. Higher | ||
values mean more exploration. Default is 0.01. | ||
|
||
- **exploration_decay** (float or None): Rate at which exploration decays over time. |
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 any float valid for this or is there a specific range? What does the value mean? The same applies to other parameters like xi
. Please go over all of them and try to be as precise as possible.
optimizer.set_gp_params(alpha=self.alpha, n_restarts_optimizer=self.n_restarts) | ||
|
||
# Use initial point as first probe | ||
probe_params = {f"param{i}": float(val) for i, val in enumerate(x0)} |
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 logic for creating a parameter dictionary as expected in bayesian-optimization is repeated a few times (here, line 128 and line 116 at least). Even though it is just one line I would extract this into a helper function.
) | ||
|
||
if optimizer.max is None: | ||
return InternalOptimizeResult( |
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 have one return statement at the end off _solve_internal_problem
n_jac_evals=0, | ||
) | ||
|
||
def _get_acquisition_function(self) -> AcquisitionFunction | 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.
I would prefer _get_acquisition_function
to be a function, not a method of the BayesOpt
class. The only thing it needs is self.acquisition_function
, so this can be passed as the argument instead of self. And then we need unit tests to cover all the code paths of that function (including the error paths).
No description provided.