-
Notifications
You must be signed in to change notification settings - Fork 42
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
Conversation
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
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 toSingleObjectiveTrustRegionBox
, 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