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.

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

TREGO using batch trust regions #778

Merged
merged 10 commits into from
Sep 13, 2023
Merged

TREGO using batch trust regions #778

merged 10 commits into from
Sep 13, 2023

Conversation

khurram-ghani
Copy link
Collaborator

@khurram-ghani khurram-ghani commented Aug 10, 2023

Related issue(s)/PRs: #773

Summary

This PR adds a TREGO implementation based on the new batch-trust-region classes and replaces the original TREGO TrustRegion implementation.

Existing notebook, unit tests and integration tests have been updated to use the new classes.

Fully backwards compatible: no

We have agreed that we don't need backwards compatibility -- see discussion below.

Previously, a TREGO rule was created like this:

rule = TrustRegion()

This will need to be replaced with the following:

rule = BatchTrustRegionBox(TREGOBox(search_space))

PR checklist

  • The quality checks are all passing
  • The bug case / new feature is covered by tests
  • Any new features are well-documented (in docstrings or notebooks)

Copy link
Collaborator

@hstojic hstojic left a comment

Choose a reason for hiding this comment

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

this is quite neat! surprisingly small amount of changes :)

Copy link
Collaborator

@hstojic hstojic left a comment

Choose a reason for hiding this comment

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

I think this is great, but if we eliminate TREGO, this would break backward compatibility, right?
I think we should still do it...
@vpicheny ? @uri-granta ?

tests/integration/test_bayesian_optimization.py Outdated Show resolved Hide resolved
trieste/acquisition/rule.py Outdated Show resolved Hide resolved
@khurram-ghani khurram-ghani mentioned this pull request Aug 21, 2023
3 tasks
Copy link
Collaborator

@vpicheny vpicheny left a comment

Choose a reason for hiding this comment

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

We don't need to be back-compatible so it's fine to use TREGOBox

Base automatically changed from khurram/trustregions to develop August 25, 2023 13:02
@khurram-ghani khurram-ghani changed the title TREGO using multi trust regions TREGO using batch trust regions Sep 7, 2023
@khurram-ghani khurram-ghani marked this pull request as ready for review September 7, 2023 16:53
@khurram-ghani khurram-ghani mentioned this pull request Sep 11, 2023
3 tasks
@khurram-ghani khurram-ghani merged commit 963f1dc into develop Sep 13, 2023
12 checks passed
@khurram-ghani khurram-ghani deleted the khurram/multi_trego branch September 13, 2023 14:12
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.

3 participants