Make method for surrogates #671
Conversation
rlars
force-pushed
the
Make_For_Surrogates
branch
5 times, most recently
from
248f969 to
8e69591
Compare
|
Remaining failing tests are due to disk space issue or fail without my changes as well. |
rlars
force-pushed
the
Make_For_Surrogates
branch
from
8e69591 to
ce9a3f1
Compare
rlars
force-pushed
the
Make_For_Surrogates
branch
from
ce9a3f1 to
7eb1e07
Compare
rlars
force-pushed
the
Make_For_Surrogates
branch
from
7eb1e07 to
cd3d122
Compare
rlars
force-pushed
the
Make_For_Surrogates
branch
from
cd3d122 to
3da10b1
Compare
jduerholt
left a comment
jduerholt
left a comment
There was a problem hiding this comment.
This looks very nice. Thanks. I only have some minor questions/comments.
| def test(surrogate_type: type[SurrogateWithMake], data_model): | ||
| if ( | ||
| type(data_model) in surrogates_skip_all | ||
| or type(surrogate_type) in surrogates_skip_all |
There was a problem hiding this comment.
Seems I missed to implement make for some surrogates (which at first glance I thought to be used internally only). And there are some more typing issues for certain classes. I will try to fix them.
There was a problem hiding this comment.
This is a shortcoming of mapping multiple surrogate data models to the same surrogate. Eventually, the test function could be adapted, but this should go along with improvements to the modelling.
|
Hi @rlars, just tell me when you are ready, then I rereview. Best, Johannes |
rlars
force-pushed
the
Make_For_Surrogates
branch
3 times, most recently
from
72aa793 to
4537e2c
Compare
… property instead.
rlars
force-pushed
the
Make_For_Surrogates
branch
from
4537e2c to
4ff4eb0
Compare
@jduerholt an you please have another look? |
|
I will have a look! |
jduerholt
left a comment
jduerholt
left a comment
There was a problem hiding this comment.
Thank you very much! I am a bit puzzled regarding the changes with the hyperconfig, can you comment on this?
Best,
Johannes
|
|
||
| hyperconfig: Optional[Hyperconfig] = None | ||
|
|
||
| @property |
There was a problem hiding this comment.
Why are we setting this not over inheritance?
There was a problem hiding this comment.
With this, TrainableSurrogate only specifies that any (non-abstract) implementation provides access to the generic hyperconfig. Therefore, TrainableSurrogate does not need to know what specific subtype of hyperconfig a surrogate implementation uses which gets rid of the generics.
hyperconfig_access is used in runners/trainable.py and other places.
| """ | ||
| return isinstance(my_type, type(ContinuousOutput)) | ||
|
|
||
| def update_hyperparameters( |
There was a problem hiding this comment.
Hmm, why are you moving it outside of the Hyperconfig? Is this necessary?
There was a problem hiding this comment.
I think this way the relationship surrogate <-> hyperconfig becomes clearer. Before, there was an inversion of responsibility where the hyperconfig would modify its surrogate in a manner which I did not anticipate.
| def _update_hyperparameters(surrogate_data, hyperparameters: pd.Series): | ||
| raise NotImplementedError( | ||
| "Ideally this would be an abstract method, but this causes problems in pydantic.", | ||
| def update_hyperparameters(self, hyperparameters: pd.Series): |
There was a problem hiding this comment.
Hmm, now I am puzzled, the update is now part of both the config and the actual model?
There was a problem hiding this comment.
Until now there were somehow reversed responsibilities, where the config would mess with the model. Now, this method (of the Hyperconfig) could likely be renamed as it is doing the validation (the code was copied multiple times before and is now at a single place satisfying DRY).
2 participants
Motivation
This PR creates make methods for surrogates, see #663
Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
For now, created a test method analogous to the one for make methods for strategies.