-
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
Filter-out local datasets when calling base-rule #805
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that if self._rules is None and not (
_num_local_models == 0 and isinstance(self._rule, EfficientGlobalOptimization)
): ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure about renaming to 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. |
||
# functions that don't expect local datasets. | ||
# Note: no need to filter out local models, as setups with local models | ||
# are handled above (i.e. we run the base rule sequentially for each subspace). | ||
if datasets is not None: | ||
_datasets = { | ||
tag: dataset | ||
for tag, dataset in datasets.items() | ||
if not LocalizedTag.from_tag(tag).is_local | ||
} | ||
else: | ||
_datasets = None | ||
points = self._rule.acquire(acquisition_space, models, _datasets) | ||
|
||
# We may modify the regions in filter_datasets later, so return a copy. | ||
state_ = BatchTrustRegion.State(copy.deepcopy(acquisition_space)) | ||
|
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.
(what's the mypy error here?)
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 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)