Filter-out local datasets when calling base-rule #805
Conversation
khurram-ghani
changed the title
There was a problem hiding this comment.
looks good I think, but perhaps would be good that @uri-granta takes a look as well
trieste/acquisition/rule.py
Outdated
| @@ -1282,7 +1282,19 @@ def state_func( | |||
| _points.append(rule.acquire(subspace, _models, _datasets)) | |||
| points = tf.stack(_points, axis=1) | |||
| else: | |||
| points = self._rule.acquire(acquisition_space, models, datasets) | |||
| # Filter out local datasets as this is the EGO rule with normal acquisition | |||
There was a problem hiding this comment.
The fact that self._rules implies (EGO + no local models) isn't obvious here. Maybe rename ._rules to
._subspace_rules and change the initialisation (above) to:
if self._rules is None and not (
_num_local_models == 0 and isinstance(self._rule, EfficientGlobalOptimization)
):?
There was a problem hiding this comment.
I am not sure about renaming to _subspace_rules, but am happy to apply De Morgan's law to the condition expression.
Also, I'll change the comment slightly to be not so definitive about being EGO. That is the case for now, but may support more rules in the future.
tests/unit/acquisition/test_rule.py
Outdated
|
|
||
| # Create a BatchTrustRegionBox instance with the mock base_rule. | ||
| subspaces = [SingleObjectiveTrustRegionBox(search_space) for _ in range(2)] | ||
| rule = BatchTrustRegionBox(subspaces, mock_base_rule) # type: ignore[var-annotated] |
There was a problem hiding this comment.
(what's the mypy error here?)
There was a problem hiding this comment.
The error is:
tests/unit/acquisition/test_rule.py:1819: error: Need type annotation for "rule" [var-annotated]
It wants the type of the rule spelled out, as it seems it can't figure that out iteself (via the base_rule). This is a standard error in most instantiations of TR rules as they have a base rule (I think). This can be avoided by using the following (for this instance). However, for unit tests I chose to be not that verbose; we instance the rules in many places.
rule: BatchTrustRegionBox[ProbabilisticModel] = BatchTrustRegionBox(subspaces, mock_base_rule)
Related issue(s)/PRs: None
Summary
Filter out local datasets when calling the EGO base rule. This will reduce the burden of filtering on most acquisition rules/functions that we currently have. If in the future we have acquisition rules/functions that need to see the local datasets, we can expand the functionality then.
Also took the opportunity to fix a very minor typing issue in
mk_batch_observer.Fully backwards compatible: yes
PR checklist