ModelWrapper #64
ModelWrapper #64
Conversation
Codecov Report
@@ Coverage Diff @@
## master #64 +/- ##
==========================================
+ Coverage 99.78% 99.79% +<.01%
==========================================
Files 16 17 +1
Lines 933 960 +27
==========================================
+ Hits 931 958 +27
Misses 2 2
Continue to review full report at Codecov.
|
GPflowOpt/acquisition/acquisition.py
Outdated
| super(Acquisition, self).__init__() | ||
| self._models = ParamList([DataScaler(m) for m in np.atleast_1d(models).tolist()]) | ||
| models = np.atleast_1d(models) | ||
| assert all(isinstance(model, (Model, ModelWrapper))for model in models) |
GPflowOpt/models.py
Outdated
|
|
||
| class ModelWrapper(Parameterized): | ||
| """ | ||
| Class for fast implementation of a wrapper for models defined in GPflow. Once wrapped, all lookups for attributes |
There was a problem hiding this comment.
lets make the first paragraph a one liner (so it is listed nicely if it ever becomes in the docs). So "Once..." is a new paragraph
GPflowOpt/models.py
Outdated
|
|
||
| def __setattr__(self, key, value): | ||
| """ | ||
| 1) If setting :attr:`wrapped` attribute, point parent to this object (the datascaler). |
GPflowOpt/models.py
Outdated
| return self.wrapped == other | ||
|
|
||
| def __str__(self, prepend=''): | ||
| return self.wrapped.__str__(prepend) |
There was a problem hiding this comment.
Useful to prepend with 'Wrapper' or something by default?
There was a problem hiding this comment.
Doesn't seem necessary, the actual method that is executed is in Parameterized so doesn't mention the model.
In the parameter naming you should see a .wrapped directive though.
There was a problem hiding this comment.
Actually, that isn't the case yet. fixing.
GPflowOpt/scaling.py
Outdated
| class DataScaler(GPModel): | ||
| class DataScaler(ModelWrapper): | ||
| """ | ||
| Model-wrapping class, primarily intended to assure the data in GPflow models is scaled. One DataScaler wraps one |
GPflowOpt/models.py
Outdated
| 1) If setting :attr:`wrapped` attribute, point parent to this object (the ModelWrapper). | ||
| 2) Setting attributes in the right objects. The following rules are processed in order: | ||
| (a) If attribute exists in wrapper, set in wrapper. | ||
| (b) If not object wrapped yet, set in wrapper. |
There was a problem hiding this comment.
what's meant with this?
that it will error out if self.wrapper is None
Following #60 , wrapping any kind of model defined in GPflow and influencing its input/output seems to become a shared use case. So I created a specific parent class which takes care of this, datascaler now is derived from it. This turns it into an actual implementation of the Decorator pattern.
The class is now derived from parameterized again. Deriving from GPmodel introduced a wide range of issues which were all more or less covered but it wasn't very nice. I also noticed that for recompiling models (I tested with the modified GPflow version which is in the PR) there was some annoying split on where certain parameters were set: this is now all covered and solved issues related to using VGP.
Note that deriving from Parameterized is not as bad as it seems at first. Because all method calls not implemented in a wrapper subclass are forwarded, and an assert is placed in the constructor which verifies its actually a model we maintain the assumptions of a model through the proxy.