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

Notebook for trust regions #777

Merged
merged 16 commits into from
Sep 11, 2023
Merged

Notebook for trust regions #777

merged 16 commits into from
Sep 11, 2023

Conversation

khurram-ghani
Copy link
Collaborator

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

Related issue(s)/PRs: #773

Summary

Added example notebook for three trust region algorithms.

Fully backwards compatible: yes

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.

its ok, but I would get back the visualisation of the TR boxes, see my comment below

docs/notebooks/multi_trust_region.pct.py Outdated Show resolved Hide resolved
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.

I am wondering if it would make sense to have a first section showing TREGO. Or ideally, TREGO, then batch-TR with Thompson sampling, and finally TurBO?

So that this notebook would give an overview of the TR features we have in trieste and would also act as an integration test?

Base automatically changed from khurram/trustregions to develop August 25, 2023 13:02
@khurram-ghani
Copy link
Collaborator Author

I am wondering if it would make sense to have a first section showing TREGO. Or ideally, TREGO, then batch-TR with Thompson sampling, and finally TurBO?

Yes, makes sense. I'll add that in the notebook using the existing versions of TREGO and TurBO. We can then migrate to the batch implementations later in the respective PRs.

@khurram-ghani khurram-ghani changed the title Notebook for multi trust regions Notebook for trust regions Sep 6, 2023
@khurram-ghani
Copy link
Collaborator Author

@vpicheny @hstojic I have updated the notebook to cover all 3 trust region algorithms.

@khurram-ghani khurram-ghani mentioned this pull request Sep 7, 2023
3 tasks
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.

looks good to me, few minor comments

docs/notebooks/trust_region.pct.py Outdated Show resolved Hide resolved
docs/notebooks/trust_region.pct.py Outdated Show resolved Hide resolved
#
# Finally, we show how to run Bayesian optimization with the `TurBO` algorithm. This is a
# trust region algorithm that uses local models and datasets to approximate the objective function
# within one trust region.
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure what the right wording is, but perhaps "within their respective trust regions"? otherwise it could sound like a single TR algorithm? ignore if you think this is sufficiently clear as is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did have slightly different wording before that implied multiple regions, but explicitly changed it to one region. The current TurBO implementation only supports one region. When that is replaced with new classes in a future PR, I can update the wording.

@khurram-ghani khurram-ghani merged commit faee54c into develop Sep 11, 2023
12 checks passed
@khurram-ghani khurram-ghani deleted the khurram/multi_tr_notebook branch September 11, 2023 16:26
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.

4 participants