Skip to content

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

spline2hg
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Jun 8, 2025

Codecov Report

Attention: Patch coverage is 79.04762% with 22 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/optimagic/optimizers/bayesian_optimizer.py 75.90% 20 Missing ⚠️
src/optimagic/config.py 60.00% 2 Missing ⚠️

❌ 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.

Files with missing lines Coverage Δ
src/optimagic/algorithms.py 86.02% <100.00%> (+0.08%) ⬆️
src/optimagic/config.py 70.12% <60.00%> (+0.68%) ⬆️
src/optimagic/optimizers/bayesian_optimizer.py 75.90% <75.90%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@janosg janosg left a 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
Copy link
Member

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.

Copy link
Member

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).
Copy link
Member

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?

Copy link
Collaborator Author

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.

Comment on lines +4071 to +4075
- **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.
Copy link
Member

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.

Copy link
Collaborator Author

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.
Copy link
Member

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)}
Copy link
Member

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(
Copy link
Member

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:
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants