Multi trust regions #773

khurram-ghani · 2023-07-26T14:45:12Z

Related issue(s)/PRs: None
Related branch: victor/trustregions

Summary

This PR adds support for multi-trust-regions acquisition rule; based-on and refactored from branch victor/trustregions.

The main classes are:

CollectionSearchSpace
- A new abstract class representing search spaces made up of a collection of sub-spaces.
TaggedMultiSearchSpace
- A concrete implementation of CollectionSearchSpace for collections where all sub-spaces have the same dimensionality. This is the class used for current trust-region rules.
TaggedProductSearchSpace
- The existing product search space class now inherits from CollectionSearchSpace.
UpdatableTrustRegion
- A new abstract class representing search spaces that can be updated or reinitialised.
BatchTrustRegion
- A new abstract class for trust region acquisition rules.
SingleObjectiveTrustRegionBox
- A concrete implementation of UpdatableTrustRegion for the algorithm implemented in victor/trustregions.
BatchTrustRegionBox
- A concrete implementation of BatchTrustRegion for the algorithm implemented in victor/trustregions.

The intention is that whenever a new trust region algorithm is implemented, in most cases, it would be sufficient to simply sub-class UpdatableTrustRegion and BatchTrustRegion.

Question: since SingleObjectiveTrustRegionBox/BatchTrustRegionBox is a specific trust-regions implementation, should it be moved to either a separate file or perhaps to a notebook? Is it generally useful?
Answer: yes these are useful and should be kept with the rules for now.

An example notebook will be added in a follow-on 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)

Provide base abstract classes for updateable search space and multi trust region rule. Implement concrete classes for box multi trust region rule.

hstojic

did a pass, few minor comments for now - but still thinking about the code in rule.py file, trying to make up my mind about updatable class - are there efficiency gains with that?

trieste/space.py

docs/notebooks/multi_trust_region.pct.py

khurram-ghani · 2023-08-01T08:54:35Z

trying to make up my mind about updatable class - are there efficiency gains with that?

@hstojic
There aren't computation efficiency gains. This is about good design patterns (encapsulation); what is easy for developers to understand and maintain. In my opinion, having a separate update-able class makes it clear what it does and keeps most of the code self contained. Only when something applies to multiple regions (e.g. maybe_reinitialise_subspaces), it exists in the rule iteself.

trieste/acquisition/rule.py

hstojic

did another round, focusing on the changes in rule

* Use generic search space type for MultiTrustRegionBox * Add some private methods to TrustRegionBox * Add kwargs to reinitialize/update * Use American spelling * Move helper function to utils

uri-granta

(a few minor initial comments while I was reminding myself of the space and optimizer work; will look at the rule next)

trieste/space.py

trieste/acquisition/utils.py

trieste/acquisition/optimizer.py

uri-granta

(Some initial rule comments in case you want to respond/chat. Will carry on looking.)

trieste/acquisition/rule.py

* Rename reinitialize to initialize * Replace get_single_model_and_dataset with generic get_value_for_tag; plus update unit tests * Add unit tests for and change get_unique_points_mask implementation as previous version erroneously identified some points as duplicates

Plus revert addition of kwargs

Plus one other minor tweak.

uri-granta

A few minor comments, but generally happy from an engineering persepctive.

trieste/space.py

trieste/acquisition/rule.py

hstojic

looks good, I have left a bunch of comments, but no major point I think - I'm mostly curious how it would look like when starting to use the new division between the updatable search space and rule class itself, in the sense of subclassing them to create new variants

tests/unit/acquisition/test_optimizer.py

trieste/acquisition/utils.py

trieste/space.py

trieste/acquisition/rule.py

trieste/acquisition/utils.py

trieste/acquisition/rule.py

Victor Picheny and others added 10 commits May 18, 2023 09:13

multiTR with notebook

905dc14

notebook

5c27203

multi local step TREGO

e46150d

Merge branch 'develop' into victor/trustregions

ceadabe

merged latest develop

990b6cd

Refactor multi trust region implementation

ebe27f4

Provide base abstract classes for updateable search space and multi trust region rule. Implement concrete classes for box multi trust region rule.

Merge remote-tracking branch 'origin/develop' into khurram/trustregions

3ff37fe

Fix formatting in notebook & remove orig TR changes

683eacc

Remove redundant new assert

809153c

Undo earlier logging change

b6ee392

khurram-ghani force-pushed the khurram/trustregions branch from 8b708a5 to b6ee392 Compare July 26, 2023 15:33

khurram-ghani and others added 5 commits July 26, 2023 16:40

Workaround isort & black clash

fcc16e2

Keep old ver of mypy happy

9f21527

Fix typo in slicing

1762d82

Add new collection space tests

535dbb1

Merge branch 'develop' into khurram/trustregions

5b97508

hstojic reviewed Jul 31, 2023

View reviewed changes

trieste/space.py Outdated Show resolved Hide resolved

docs/notebooks/multi_trust_region.pct.py Outdated Show resolved Hide resolved

hstojic reviewed Aug 1, 2023

View reviewed changes

trieste/acquisition/rule.py Outdated Show resolved Hide resolved

hstojic reviewed Aug 1, 2023

View reviewed changes

khurram-ghani added 3 commits August 2, 2023 16:14

Address feedback plus other small changes

c4ce7ab

* Use generic search space type for MultiTrustRegionBox * Add some private methods to TrustRegionBox * Add kwargs to reinitialize/update * Use American spelling * Move helper function to utils

Merge remote-tracking branch 'origin/develop' into khurram/trustregions

20b5687

Add TrustRegionBox/UpdateableSearchSpace unittests

0c38efb

uri-granta reviewed Aug 3, 2023

View reviewed changes

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

uri-granta reviewed Aug 3, 2023

View reviewed changes

trieste/acquisition/rule.py Outdated Show resolved Hide resolved

khurram-ghani added 3 commits August 3, 2023 15:00

Address feedback for space changes

309ef09

Create subspaces outside the rule

c2abf11

Plus revert addition of kwargs

khurram-ghani and others added 5 commits August 8, 2023 10:06

Merge branch 'develop' into khurram/trustregions

a313bf8

Add shape-checking/docstring for mask func

0706656

Plus one other minor tweak.

Add check_shapes, update docstrings & add tag test

9def562

Remove notebook, to be added in separate PR

0973630

Add deepcopy test

01f370b

khurram-ghani marked this pull request as ready for review August 9, 2023 09:13

khurram-ghani requested review from uri-granta and hstojic August 9, 2023 09:14

khurram-ghani added 2 commits August 9, 2023 12:37

Merge remote-tracking branch 'origin/develop' into khurram/trustregions

dad0936

Check TR copy is deep

66b4aa4

khurram-ghani mentioned this pull request Aug 9, 2023

Notebook for trust regions #777

Merged

3 tasks

Improve TR box and test slightly

9bb6a80

khurram-ghani mentioned this pull request Aug 10, 2023

TREGO using batch trust regions #778

Merged

3 tasks

uri-granta reviewed Aug 14, 2023

View reviewed changes

trieste/space.py Show resolved Hide resolved

trieste/space.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 Outdated Show resolved Hide resolved

hstojic reviewed Aug 17, 2023

View reviewed changes

khurram-ghani added 6 commits August 21, 2023 13:47

Address Uri's latest comments

4924557

Move get_value_for_tag to more general utils/misc

f0e125d

Remove redundant [] checks

3daa81f

Add fixture for default search space type

0b19246

Add test for optim multiple boxes as a batch

044f07c

Rename classes and expand some docstrings

8ca39e9

uri-granta approved these changes Aug 24, 2023

View reviewed changes

Choose longer class name

ef62eeb

khurram-ghani mentioned this pull request Aug 24, 2023

Investigate improvements to the get-unique-points algorithm #781

Closed

vpicheny reviewed Aug 24, 2023

View reviewed changes

trieste/acquisition/rule.py Outdated Show resolved Hide resolved

Merge branch 'develop' into khurram/trustregions

5634c85

khurram-ghani mentioned this pull request Aug 25, 2023

Batch trust regions should use latest query point #782

Closed

Rename updatable class name

8f17839

khurram-ghani merged commit 93f44b3 into develop Aug 25, 2023

khurram-ghani deleted the khurram/trustregions branch August 25, 2023 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi trust regions #773

Multi trust regions #773

khurram-ghani commented Jul 26, 2023 •

edited

Loading

hstojic left a comment

khurram-ghani commented Aug 1, 2023

hstojic left a comment

uri-granta left a comment

uri-granta left a comment

uri-granta left a comment

hstojic left a comment

Multi trust regions #773

Multi trust regions #773

Conversation

khurram-ghani commented Jul 26, 2023 • edited Loading

Summary

PR checklist

hstojic left a comment

Choose a reason for hiding this comment

khurram-ghani commented Aug 1, 2023

hstojic left a comment

Choose a reason for hiding this comment

uri-granta left a comment

Choose a reason for hiding this comment

uri-granta left a comment

Choose a reason for hiding this comment

uri-granta left a comment

Choose a reason for hiding this comment

hstojic left a comment

Choose a reason for hiding this comment

khurram-ghani commented Jul 26, 2023 •

edited

Loading