Transforms #355
Transforms #355
Conversation
| from bofire.data_models.domain.api import Domain | ||
|
|
||
|
|
||
| class Transform: |
There was a problem hiding this comment.
I think we do not need here a distinction between data model and functional model, as the transform does not need any non standard dependecies, or? So one can just implement it for the datamodel. The same could be done also for the conditions, or?
We should also have a loo here: https://github.com/facebook/Ax/tree/main/ax/modelbridge/transforms and think of making the transforms more prominent, as transforms of the domain, the candidates and the experiments could be also useful in other scenarios. Have a look for this to the PR of @Osburg, where he transform the complete domain between bound of -1 and 1. Having this kinds of transform, that could also be chained behind it each other could make sense, they would always take in the domain, experiments and candidates and return the transformed versions, what do you think?
There was a problem hiding this comment.
Moved to data models as discussed.
There was a problem hiding this comment.
@jduerholt can you have a look at the invalid/valid spec-stuff? I did something wrong and don't really get it.
|
I took a quick look at this. I think that the name RemoveTransform is a little bit confusing because I would expect it to remove the transform rather than be a transform that removes something. What do you think of DropDataTransform or ExcludeTransform? |
Yes! As so often a great naming suggestion from you! |
| from pydantic import Field, field_validator | ||
| from typing_extensions import Annotated | ||
|
|
||
| from bofire.data_models.base import BaseModel | ||
| from bofire.data_models.domain.api import Domain |
There was a problem hiding this comment.
Why do you have this extra class and not just put the abstract method in Condition?
There was a problem hiding this comment.
I prefer abstract classes to be really abstract and NOT to be mixed with data members or implementations.
[edit] not was missing[/edit]
| self.stratgies = [_map(s.strategy_data) for s in data_model.steps] | ||
| self.conditions = [s.condition for s in data_model.steps] | ||
| self.transforms = [ | ||
| s.transform and transforms.map(s.transform) for s in data_model.steps |
There was a problem hiding this comment.
What is the and doing? I do not get it :D
There was a problem hiding this comment.
What is the
anddoing? I do not get it :D
If s.transform is None we keep None. If it is not None the last value is evaluated and returned.
assert (None and 1) is None
assert (None or 1) == 1
| return domain | ||
|
|
||
| def untransform_experiments(self, experiments: pd.DataFrame) -> pd.DataFrame: | ||
| return experiments |
There was a problem hiding this comment.
I would also opt for an untransform_candidates
There was a problem hiding this comment.
We only untransform candidates. And only the new candidates that have been proposed. For the other things, the inputs are the untransformed versions, no?
Co-authored-by: Johannes P. Dürholt <[email protected]>
Co-authored-by: Johannes P. Dürholt <[email protected]>
There was a test in the stepwise-file. I moved it to a separate file and extended it. |
There was a problem hiding this comment.
Looks good to me, last minor thing would be to not have test_transforms.py under tests/bofire/strategegies but in a new folder tests/bofire/stransforms.py so that we reflect the correct folder structure. If one is very strict we would also need the test_conditions in the data_models part of the test suite.
Thanks for the review. Since transforms are tight to the stepwise strategy I will leave the tests there. We can move them later. |
Implement Transforms
For instance, they they can be applied to steps in the step-wise strategy. This way you can for example change data or candidates the strategies for certain steps or restrict the domain.