Categorical trust regions #865
Conversation
| @@ -1624,7 +1626,13 @@ def __init__( | |||
| self._y_min = tf.constant(np.inf, dtype=self.location.dtype) | |||
|
|
|||
| def _init_eps(self) -> None: | |||
| self.eps = self._zeta * (self.global_search_space.upper - self.global_search_space.lower) | |||
| if not isinstance(self.global_search_space, HasOneHotEncoder): | |||
There was a problem hiding this comment.
Using an isinstance check in this file to detect categorical spaces makes me a bit uncomfortable, as it forces developers to have to inherit from HasOneHotEncoder, and it feels too much of a special case. I think our original design decision was to use a property like is_categorical to do this. I don't have a strong objection, but just wanted to highlight that. I think we already discussed this before, but can't remeber the conclusion.
There was a problem hiding this comment.
Remind me: are there any categorical spaces (where we would wish to use Hamming distances) that are also numerically bounded? Because looking at it now, it seems much more natural to write
| if not isinstance(self.global_search_space, HasOneHotEncoder): | |
| if self.global_search_space.has_bounds: |
Certainly we shouldn't calculate eps for unbounded spaces.
There was a problem hiding this comment.
I don't think so. We won't have categorical spaces that have bounds. However, we could potentially have the reverse, i.e. spaces that are not bounded and are also not categorical.
There was a problem hiding this comment.
Well we can't use eps as written for unbounded spaces, even if they're not categorical, as it uses the bounds. Is there any reason not to default to Hamming distance for those cases, at least for now?
There was a problem hiding this comment.
That seems fine to me. @vpicheny what do you think?
trieste/acquisition/rule.py
Outdated
| # use Hamming distance for categorical spaces | ||
| return tf.math.reduce_sum( | ||
| tf.where(tf.expand_dims(points, -2) == tf.expand_dims(points, -3), 0, 1), | ||
| axis=-1, | ||
| keepdims=True, # (keep last dim for distance calculation below) | ||
| ) # [num_points, num_points, 1] | ||
| else: |
There was a problem hiding this comment.
I think maybe we should add more of an explanation here, as the categorical and numerical cases are slightly inconsistent.
The size of the last dimension for the numerical case is D, i.e. the distance in each dimension is calculated separately. Each dimesion is then separately tested against distance in _get_points_within_distance and it selects neighbors if all dimensions are within distance (i.e. reduce_all below).
For the categorical case the last dimension is 1 as we do a reduce_sum. I can see why that is, as we want to effectively do a reduce_any in _get_points_within_distance, i.e. the neighbors are selected if they are within distance in any dimension.
So we can add an explanation, or alternatively do the selection below and explicitly add a reduce_any.
Related issue(s)/PRs:
Summary
Follows on from #864.
Fully backwards compatible: yes / no
PR checklist