feat: New surrogate models API #669
Conversation
| @@ -0,0 +1,102 @@ | |||
| # Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Can you think of a different name than contributed?
This sounds like we discourage contributions on the BO side.
| cand_lst = [ | ||
| hp_ranges.to_ndarray(config) for config in state.pending_configurations() | ||
| ] | ||
| cand_lst = [] |
There was a problem hiding this comment.
This change needs some comment, I am not sure this works out.
There was a problem hiding this comment.
This was commited by mistake, already removed in the new commit.
| @@ -174,31 +174,3 @@ def transform_state_to_data( | |||
| targets = np.vstack([targets * np.ones((1, num_fantasy_samples))] + fanta_lst) | |||
| features = np.vstack([features] + cand_lst) | |||
| return TransformedData(features, targets, mean, std) | |||
|
|
|||
|
|
|||
| class EstimatorFromTransformedData(Estimator): | |||
There was a problem hiding this comment.
Why remove this one? I thought this is something we wanted to have
There was a problem hiding this comment.
EstimatorWrapper now implements this function, its a rename + move rather than a removal.
| raise NotImplementedError() | ||
|
|
||
|
|
||
| class ContributedPredictorWrapper(BasePredictor): |
There was a problem hiding this comment.
I think this one should move to models/contributed_estimator.py (except that "contributed" should become something else).
This is like for GP: The wrappers are in bayesopt/models (both estimator and predictor in the same wrapper code file), the code is in bayesopt/gpautograd
There was a problem hiding this comment.
Ok makes sense, will move.
There was a problem hiding this comment.
And note that your code in bayesopt/contributed need not adhere to the estimator / predictor split, but you can keep everything in a single class. The split into two is only required one level up, in the wrappers.
That's what I did for the GP code, and also what we discussed.
There was a problem hiding this comment.
Thats a good point but I wanted to keep them separated to make it more streamlined. This way we rely on the decoupled view through all the levels and I think it will be easier to follow for newer contributors. I know that I was confused by couplig/decoupling in GP code the first time I saw it.
| return False | ||
|
|
||
|
|
||
| class ContributedEstimatorWrapper(Estimator): |
There was a problem hiding this comment.
I think this should also move to models/contributed_estimator.py, and why not inherit from EstimatorFromTransformedData ?
There was a problem hiding this comment.
This is just a rename of EstimatorFromTransformedData. I wanted to make estimator/predictor consistant with each other.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #669 +/- ##
==========================================
+ Coverage 65.98% 66.13% +0.15%
==========================================
Files 381 386 +5
Lines 26917 27003 +86
==========================================
+ Hits 17760 17859 +99
+ Misses 9157 9144 -13
☔ View full report in Codecov by Sentry. |
|
OK, I think my main comment would be that only the wrapper code (in |
| input points ``inputs``. By default: | ||
|
|
||
| * "mean": Predictive means. | ||
| - "std": Predictive stddevs, shape ``(n,)`` |
There was a problem hiding this comment.
But you could also drop this comment, because it is kind of just what the superclass has.
There was a problem hiding this comment.
Good point, I wated to change it for both but dropped it. Adjusted this and the base class now.
Implementation of the new surrogate models API as discussed during the Surrogate-models-in-Syne-Tune meeting.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.