bayesian-optimizer #602
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.