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

Local models and datasets #788

Merged
merged 38 commits into from
Dec 14, 2023
Merged

Local models and datasets #788

merged 38 commits into from
Dec 14, 2023

Conversation

khurram-ghani
Copy link
Collaborator

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

Related issue(s)/PRs: #782

Summary

This PR adds support for local models and datasets. The following scenarios are supported:

  • global model and global dataset -- as previously
  • local models and local datasets -- one-to-one mapping
  • global model and local datasets -- many-to-one mapping

The initial dataset can be a global dataset sampled from the whole search space. This data will be replicated to each of the regions on the first iteration and subsequently each region will have an associated local dataset. For batch-TR algorithm, the dataset for each region are filtered after each iteration to only contain the points in the region (but TREGO doesn't do this).

Note: this replication of initial data can potentially cause an issue when a global model is being used, as the points may be repeated. This will only be an issue if regions overlap and both contain that initial data-point (as filtering would otherwise remove duplicates). The main way to avoid this issue is to provide local initial datasets, instead of a global initial dataset.

The trust_region notebook contains a new temporary TEST section, just to show how local models can be used in the notebook. It is worth noting in the gif that the query-points are filtered to be only inside the boxes for each iteration. This section is only for demonstration and will be removed before merging this PR. A follow-on PR will update the TURBO section to use local models and demonstrate this functionality.

Fully backwards compatible: no

The BatchTrustRegion rule acquisition returns rank 3 points, instead of rank 2 as for other rules (and previous trust-region rules). This means the users should use the new batched observer with this rule. That is already taken care of in BayesianOptimizer. However, with AskTellOptimizer the users should use the batched observer as follows:

from trieste.objectives.utils import mk_batch_observer

observer = ...
batch_observer = mk_batch_observer(observer)

new_points = ask_tell.ask()
new_data = batch_observer(new_points)
ask_tell.tell(new_data)

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

@uri-granta uri-granta left a comment

Choose a reason for hiding this comment

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

Some initial comments on the first couple of files. Will hopefully have more time to continue looking at this.

trieste/objectives/utils.py Outdated Show resolved Hide resolved
trieste/objectives/utils.py Outdated Show resolved Hide resolved
trieste/utils/misc.py Outdated Show resolved Hide resolved
trieste/utils/misc.py Outdated Show resolved Hide resolved
trieste/utils/misc.py Show resolved Hide resolved
trieste/utils/misc.py Outdated Show resolved Hide resolved
trieste/utils/misc.py Outdated Show resolved Hide resolved
trieste/utils/misc.py Outdated Show resolved Hide resolved
@uri-granta uri-granta self-requested a review October 31, 2023 10:40
Copy link
Collaborator

@uri-granta uri-granta left a comment

Choose a reason for hiding this comment

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

Still need to look at the tests but looking good so far! Various small comments, suggestions and questions.

trieste/utils/misc.py Outdated Show resolved Hide resolved
trieste/acquisition/utils.py Show resolved Hide resolved
trieste/bayesian_optimizer.py Outdated Show resolved Hide resolved
trieste/bayesian_optimizer.py Show resolved Hide resolved
trieste/bayesian_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
trieste/acquisition/rule.py Show resolved Hide resolved
Copy link
Collaborator

@uri-granta uri-granta left a comment

Choose a reason for hiding this comment

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

(a few more comments)

trieste/ask_tell_optimization.py Outdated Show resolved Hide resolved
tests/unit/test_ask_tell_optimization.py Outdated Show resolved Hide resolved
tests/unit/test_ask_tell_optimization.py Outdated Show resolved Hide resolved
tests/unit/objectives/test_utils.py Outdated Show resolved Hide resolved
tests/integration/test_ask_tell_optimization.py Outdated Show resolved Hide resolved
docs/notebooks/trust_region.pct.py Outdated 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.

I haven't gone through it in details, there is a lot going on here...

my biggest comment is on adding additional complexity to ask-tell and bayesian_optimizer - do we really need to take care of the local models there?
very few acq rules will make use of local models, why not taking care of datasets and model training in the acquisition rule itself, in those few that make use of it?

trieste/ask_tell_optimization.py Show resolved Hide resolved
trieste/objectives/utils.py Outdated Show resolved Hide resolved
trieste/acquisition/rule.py Outdated 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.

ok after a discussion

@khurram-ghani khurram-ghani merged commit f766953 into develop Dec 14, 2023
12 checks passed
@khurram-ghani khurram-ghani deleted the khurram/local_models branch December 14, 2023 12:34
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