Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add search space class for BayesOpt #355

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Add search space class for BayesOpt #355

merged 2 commits into from
Aug 10, 2023

Conversation

Thomas-Christie
Copy link
Contributor

Added an AbstractSearchSpace abstract base class for the BayesOpt library, as well as a concrete implementation in the form of a ContinuousSearchSpace class. Tests have also been added.

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've formatted the new code by running poetry run pre-commit run --all-files --show-diff-on-failure before committing.
  • I've added tests for new code.
  • I've added docstrings for the new code.

Description

Starting to add fully tested Bayesian Optimisation functionality. This first PR adds a class for defining (and sampling from) search spaces, over which optimisation is performed.

Issue Number: N/A

@Thomas-Christie
Add search space class for BayesOpt
a723952 
Added an `AbstractSearchSpace` abstract base class for the BayesOpt library, as well as a concrete implementation in the form of a `ContinuousSearchSpace` class. Tests have also been added.
@Thomas-Christie Thomas-Christie self-assigned this Aug 9, 2023
henrymoss
henrymoss approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@henrymoss henrymoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Just a couple comments

@Thomas-Christie
Minor additions to search space
d487bd6 
- Check that `lower_bounds` and `upper_bounds` are of same dtype.
- Allow some dimensions of `lower_bounds` to be equal to `upper_bounds`
  as users may wish to keep certain dimensions fixed.
@thomaspinder
Copy link
Collaborator

@henrymoss @Thomas-Christie - do we want the target branch of this PR to be main, or should we create an intermediary BO branch. Once the BO framework is in a minimal working state, we can then bulk merge that into main. Thoughts?

@henrymoss
Copy link
Contributor

@thomaspinder we have a working full version of BO that we like now in another branch. I jsut suggested to Thomas that he chopped it up into a few manageable bits, e.g. just the search space PR

@Thomas-Christie
Copy link
Contributor Author

Yes @thomaspinder, @henrymoss and I decided we'd split up PR #338 into a few fully tested PRs for ease of review. I'm happy to just merge into main to simplify things, as these PRs should all be getting added within the next week or two, but am happy to merge onto a separate BO branch if you'd prefer us not to merge onto main for now.

@Thomas-Christie Thomas-Christie changed the base branch from main to tchristie/bo August 10, 2023 12:27
@Thomas-Christie
Copy link
Contributor Author

Will merge this PR onto the tchristie/bo branch.

@Thomas-Christie Thomas-Christie merged commit d215c43 into JaxGaussianProcesses:tchristie/bo Aug 10, 2023
19 checks passed
@Thomas-Christie Thomas-Christie mentioned this pull request Sep 8, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henrymoss henrymoss henrymoss approved these changes

@thomaspinder thomaspinder Awaiting requested review from thomaspinder

Assignees

@Thomas-Christie Thomas-Christie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Thomas-Christie @thomaspinder @henrymoss