random scalarization - part 1#689
Conversation
uri-granta
left a comment
uri-granta
left a comment
There was a problem hiding this comment.
Looks good. Some initial comments. Will obviously need more tests before it can be checked in.
| from trieste.types import Tag, TensorType | ||
|
|
||
|
|
||
| IdealSpecCallable = Callable[..., TensorType] |
There was a problem hiding this comment.
Why not make this precise?
| IdealSpecCallable = Callable[..., TensorType] | |
| IdealSpecCallable = Callable[[Mapping[Tag, HasTrajectorySampler], datasets: Mapping[Tag, Dataset]], TensorType] |
| ideal = tf.cast(self._ideal_spec(models, datasets), dtype=dtype) | ||
| else: | ||
| ideal = tf.cast(self._ideal_spec, dtype=dtype) | ||
| tf.debugging.assert_shapes([(ideal, (self._num_objectives, 1))]) |
| def _get_ideal( | ||
| self, models: Mapping[Tag, HasTrajectorySampler], datasets: Mapping[Tag, Dataset] | ||
| ) -> TensorType: | ||
| dtype = self._infer_dtype(datasets) |
There was a problem hiding this comment.
This is a bit clunky: e.g. what happens if datasets is empty, or has inconsistent dtypes, or if the dtype changes between calls? Can't we just insist that the ideal_spec returns the correct dtype?
If that's a problem, maybe:
- make _infer_dtype more resilient by giving an error if datasets is empty or inconsistent?
- save the dtype when you call prepare and check it hasn't changed in update?
- add a dtype parameter to _get_ideal like _sample_weights (or alternatively move the casting outside)?
| ) -> None: | ||
| """ | ||
| Generate all the internal variables on initialization. For example, weights in a linear | ||
| weighted sum scalarization could be sampled. |
There was a problem hiding this comment.
Docstring should mention parameters
| return ( | ||
| f"Chebyshev({self._batch_size!r}," | ||
| f"{self._num_objectives!r}," | ||
| f"{self._ideal_spec.__name__}," |
There was a problem hiding this comment.
You could easily remove the code duplication if you want:
| f"{self._ideal_spec.__name__}," | |
| + (f"{self._ideal_spec.__name__}," if callable(self._ideal_spec) else f"{self._ideal_spec!r},") + |
| self, models: Mapping[Tag, HasTrajectorySampler], datasets: Mapping[Tag, Dataset] | ||
| ) -> None: | ||
| dtype = self._infer_dtype(datasets) | ||
| self._weights = tf.Variable( # pylint: disable=attribute-defined-outside-init |
There was a problem hiding this comment.
I'm worried that this would create tensorflow compilation issues, if the variables don't exist straight after initialisation. I think elsewhere we've initialised similar variables to dummy values, and (if need be) tracked initialisation with a self._initialized = tf.Variable(False) variable.
|
|
||
| self._scalarizer.prepare(models, datasets) | ||
|
|
||
| self._trajectory_sampler = { # pylint: disable=attribute-defined-outside-init |
There was a problem hiding this comment.
I think this is less likely to cause issues than the attribute-defined-outside-init Variables below, but would still be nice to initialise these to empty dicts and update them in place here. Similarly, you should declare self._negated_trajectory as an Optional[TrajectorySampler] and initialise it to None.
| -3.2095, | ||
| id="BatchMonteCarloExpectedHypervolumeImprovement/4", | ||
| ), | ||
| # pytest.param( |
There was a problem hiding this comment.
You probably don't want to check this in :-)
2 participants
this PR implements Random scalarization acquisition builder and function, interface for scalarization functions and one example of a scalarization function
part 2 will follow with a notebook
part 3 will follow with some functions for adaptive ideal points (and possible 1-2 more scalarization functions)
still at a draft stage to collect opinions on the design
(tests are missing and docs are not ironed out)
main questions about the design:
HasReparamSamplerModelStack)random_scalarizationis more of a container, most of the work is done byScalarizer. Not sure what's the best approach here, perhaps we should integrate the scalarizer in therandom_scalarization, but then it might be more difficult to use a different scalarization function. I'm also not sure how this separation affects retracing.@vpicheny @uri-granta @henrymoss any thoughts on these?