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

Updatable discrete trust region #825

Merged
merged 16 commits into from
Mar 19, 2024
Merged

Updatable discrete trust region #825

merged 16 commits into from
Mar 19, 2024

Conversation

khurram-ghani
Copy link
Collaborator

Related issue(s)/PRs: None

Summary

This PR adds a new updatable discrete trust region. This builds on the the fixed-point discrete TR added in a previous PR. The new class SingleObjectiveTrustRegionDiscrete is similar in behaviour to SingleObjectiveTrustRegionBox, but for a discrete search space.

The mixed search spaces notebook and integ tests now use this class, instead of FixedPointTrustRegionDiscrete from the previous PR.

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.

I did a quick round now, I think I want to step through the code for some of the stuff, but don't have time right now - I had some comments though that can be addressed already

docs/notebooks/mixed_search_spaces.pct.py Show resolved Hide resolved
trieste/acquisition/optimizer.py Outdated Show resolved Hide resolved
trieste/acquisition/optimizer.py Outdated Show resolved Hide resolved
trieste/acquisition/rule.py Outdated Show resolved Hide resolved
trieste/acquisition/rule.py Outdated Show resolved Hide resolved
trieste/acquisition/rule.py Show resolved Hide resolved
trieste/acquisition/rule.py Show resolved Hide resolved
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.

stepped through the code in detail and have few more comments, see below

tests/unit/acquisition/test_optimizer.py Show resolved Hide resolved
tests/unit/acquisition/test_rule.py Show resolved Hide resolved
trieste/acquisition/rule.py Show resolved Hide resolved
trieste/acquisition/rule.py Show resolved Hide resolved
trieste/acquisition/rule.py Show resolved Hide resolved
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

# section of the [trust region Bayesian optimization notebook](trust_region.ipynb). That notebook
# creates trust regions of type `SingleObjectiveTrustRegionBox`. Here, we create trust regions that
# are a product of a discrete and a continuous trust sub-region with
# `UpdatableTrustRegionProduct`. The continuous part `SingleObjectiveTrustRegionBox` is the same as
Copy link
Collaborator

@vpicheny vpicheny Mar 18, 2024

Choose a reason for hiding this comment

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

The text is good, but I am wondering (prob not for this PR) if it would be better to do this automatically? As in, it is probably a bit of a headache for the use to know that if they have a mixed input space then they must use a TrustRegionProduct. Ideally they would use SingleObjectiveTR and we dispatch it automatically to the right kind?

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'll think about this for a separate PR.

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.

looks good overall! I was wondering if we could have abstracted things a bit more since this new class has a very bespoke TR scheme (e.g. init_eps is hard-coded, etc.), but this might not be possible.

@khurram-ghani khurram-ghani merged commit 07b264b into develop Mar 19, 2024
12 checks passed
@khurram-ghani khurram-ghani deleted the khurram/discrete_tr branch March 19, 2024 15:01
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.

None yet

3 participants