From 801a51a308e707e33089ecec5849908df2f25af8 Mon Sep 17 00:00:00 2001 From: Joachim van der Herten Date: Fri, 18 Aug 2017 15:12:35 +0200 Subject: [PATCH 1/4] Recompilation fix, by inserting a hook object in highest_parent --- GPflowOpt/acquisition/acquisition.py | 4 ++-- GPflowOpt/models.py | 29 ++++++++++++++++++++++++++++ GPflowOpt/scaling.py | 4 ++-- testing/test_acquisition.py | 25 ++++++++++++++++++++++-- testing/test_datascaler.py | 1 + testing/test_modelwrapper.py | 26 +++++++++++++++++++++++++ 6 files changed, 83 insertions(+), 6 deletions(-) diff --git a/GPflowOpt/acquisition/acquisition.py b/GPflowOpt/acquisition/acquisition.py index 69d4b80..cdaf24d 100644 --- a/GPflowOpt/acquisition/acquisition.py +++ b/GPflowOpt/acquisition/acquisition.py @@ -142,7 +142,7 @@ def models(self): :return: list of GPflow models """ - return self._models + return self._models.sorted_params @property def data(self): @@ -263,7 +263,7 @@ def _optimize_models(self): @Acquisition.models.getter def models(self): - return ParamList([model for acq in self.operands for model in acq.models.sorted_params]) + return [model for acq in self.operands for model in acq.models] def enable_scaling(self, domain): for oper in self.operands: diff --git a/GPflowOpt/models.py b/GPflowOpt/models.py index 0cdda58..7ddab1d 100644 --- a/GPflowOpt/models.py +++ b/GPflowOpt/models.py @@ -15,6 +15,28 @@ from GPflow.model import Model +class ParentHook(object): + def __init__(self, highest_parent, highest_model): + self._hp = highest_parent + self._hm = highest_model + + def __getattr__(self, item): + if item is '_needs_recompile': + return getattr(self._hm, item) + return getattr(self._hp, item) + + def __setattr__(self, key, value): + if key in ['_hp', '_hm']: + object.__setattr__(self, key, value) + return + if key is '_needs_recompile': + setattr(self._hm, key, value) + if value: + self._hp._kill_autoflow() + else: + setattr(self._hp, key, value) + + class ModelWrapper(Parameterized): """ Class for fast implementation of a wrapper for models defined in GPflow. @@ -25,6 +47,7 @@ class ModelWrapper(Parameterized): AutoFlow methods are influenced following this pattern, the original AF storage (if existing) is unaffected and a new storage is added to the subclass. """ + def __init__(self, model): """ :param model: model to be wrapped @@ -45,6 +68,7 @@ def __getattr__(self, item): method = item[1:].rstrip('_AF_storage') if method in dir(self): raise AttributeError("{0} has no attribute {1}".format(self.__class__.__name__, item)) + return getattr(self.wrapped, item) def __setattr__(self, key, value): @@ -90,3 +114,8 @@ def __eq__(self, other): def name(self): name = super(ModelWrapper, self).name return ".".join([name, str.lower(self.__class__.__name__)]) + + @property + def highest_parent(self): + original_hp = super(ModelWrapper, self).highest_parent + return original_hp if isinstance(original_hp, ParentHook) else ParentHook(original_hp, self) diff --git a/GPflowOpt/scaling.py b/GPflowOpt/scaling.py index a8dee0e..cad4179 100644 --- a/GPflowOpt/scaling.py +++ b/GPflowOpt/scaling.py @@ -13,7 +13,6 @@ # limitations under the License. from GPflow.param import DataHolder, AutoFlow -from GPflow.model import GPModel from GPflow import settings import numpy as np from .transforms import LinearTransform, DataTransform @@ -53,6 +52,7 @@ class DataScaler(ModelWrapper): required, it is the responsibility of the implementation to rescale the hyperparameters. Additionally, applying hyperpriors should anticipate for the scaled data. """ + def __init__(self, model, domain=None, normalize_Y=False): """ :param model: model to be wrapped @@ -90,7 +90,7 @@ def input_transform(self, t): :param t: :class:`.DataTransform` object: the new input transform. """ - assert(isinstance(t, DataTransform)) + assert (isinstance(t, DataTransform)) X = self.X.value self._input_transform.assign(t) self.X = X diff --git a/testing/test_acquisition.py b/testing/test_acquisition.py index 4a2ebf3..c351766 100644 --- a/testing/test_acquisition.py +++ b/testing/test_acquisition.py @@ -22,7 +22,6 @@ def vlmop2(x): return np.hstack((y1, y2)) - class _TestAcquisition(object): """ Defines some basic verifications for all acquisition functions. Test classes can derive from this @@ -258,7 +257,7 @@ def test_object_integrity(self): for oper in self.acquisition.operands: self.assertTrue(isinstance(oper, GPflowOpt.acquisition.Acquisition), msg="All operands should be an acquisition object") - self.assertListEqual(self.acquisition.models.sorted_params, self.models) + self.assertListEqual(self.acquisition.models, self.models) def test_data(self): super(_TestAcquisitionAggregation, self).test_data() @@ -440,3 +439,25 @@ def test_multi_aggr(self): joint = first * second self.assertIsInstance(joint, GPflowOpt.acquisition.AcquisitionProduct) self.assertListEqual(joint.operands.sorted_params, [acq1, acq2, acq3, acq4]) + + +class TestRecompile(unittest.TestCase): + """ + Regression test for #37 + """ + def test_vgp(self): + domain = GPflowOpt.domain.UnitCube(2) + X = GPflowOpt.design.RandomDesign(10, domain).generate() + Y = np.sin(X[:,[0]]) + m = GPflow.vgp.VGP(X, Y, GPflow.kernels.RBF(2), GPflow.likelihoods.Gaussian()) + acq = GPflowOpt.acquisition.ExpectedImprovement(m) + self.assertFalse(m._needs_recompile) + acq.evaluate(GPflowOpt.design.RandomDesign(10, domain).generate()) + self.assertTrue(hasattr(acq, '_evaluate_AF_storage')) + + Xnew = GPflowOpt.design.RandomDesign(5, domain).generate() + Ynew = np.sin(Xnew[:,[0]]) + acq.set_data(np.vstack((X, Xnew)), np.vstack((Y, Ynew))) + self.assertFalse(hasattr(acq, '_needs_recompile')) + self.assertFalse(hasattr(acq, '_evaluate_AF_storage')) + acq.evaluate(GPflowOpt.design.RandomDesign(10, domain).generate()) diff --git a/testing/test_datascaler.py b/testing/test_datascaler.py index 07ffb9c..bf7c981 100644 --- a/testing/test_datascaler.py +++ b/testing/test_datascaler.py @@ -103,3 +103,4 @@ def test_predict_scaling(self): fs = n.predict_density(Xt, Yt) np.testing.assert_allclose(fr, fs, rtol=1e-2) + diff --git a/testing/test_modelwrapper.py b/testing/test_modelwrapper.py index 439858e..af1b7b1 100644 --- a/testing/test_modelwrapper.py +++ b/testing/test_modelwrapper.py @@ -118,6 +118,32 @@ def test_name(self): n = MethodOverride(self.simple_model()) self.assertEqual(n.name, 'unnamed.methodoverride') + def test_parent_hook(self): + m = self.simple_model() + m.optimize(maxiter=5) + w = GPflowOpt.models.ModelWrapper(m) + self.assertTrue(isinstance(m.highest_parent, GPflowOpt.models.ParentHook)) + self.assertEqual(m.highest_parent._hp, w) + self.assertEqual(m.highest_parent._hm, w) + w2 = GPflowOpt.models.ModelWrapper(w) + self.assertEqual(m.highest_parent._hp, w2) + self.assertEqual(m.highest_parent._hm, w2) + + p = GPflow.param.Parameterized() + p.model = w2 + self.assertEqual(m.highest_parent._hp, p) + self.assertEqual(m.highest_parent._hm, w2) + + p.predictor = self.simple_model() + p.predictor.predict_f(p.predictor.X.value) + self.assertTrue(hasattr(p.predictor, '_predict_f_AF_storage')) + self.assertFalse(m._needs_recompile) + m.highest_parent._needs_recompile = True + self.assertFalse('_needs_recompile' in p.__dict__) + self.assertFalse('_needs_recompile' in w.__dict__) + self.assertFalse('_needs_recompile' in w2.__dict__) + self.assertTrue(m._needs_recompile) + self.assertFalse(hasattr(p.predictor, '_predict_f_AF_storage')) From 336c708f549b9555017a6d474d1345c5e4610b99 Mon Sep 17 00:00:00 2001 From: Joachim van der Herten Date: Mon, 21 Aug 2017 14:47:06 +0200 Subject: [PATCH 2/4] Adding documentation for ParentHook --- GPflowOpt/models.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/GPflowOpt/models.py b/GPflowOpt/models.py index 7ddab1d..d1c1712 100644 --- a/GPflowOpt/models.py +++ b/GPflowOpt/models.py @@ -16,6 +16,13 @@ class ParentHook(object): + """ + Temporary solution for fixing the recompilation issues (#37, GPflow issue #442). + + An object of this class is returned when highest_parent is called on a model, which holds references to the highest + parentable, as well as the highest model class. When setting the needs recompile flag, this is intercepted and + performed on the model. At the same time, kill autoflow is called on the highest parent. + """ def __init__(self, highest_parent, highest_model): self._hp = highest_parent self._hm = highest_model @@ -115,7 +122,10 @@ def name(self): name = super(ModelWrapper, self).name return ".".join([name, str.lower(self.__class__.__name__)]) - @property + @Parameterized.highest_parent.getter def highest_parent(self): + """ + Returns an instance of the ParentHook instead of the usual reference to a Parentable. + """ original_hp = super(ModelWrapper, self).highest_parent return original_hp if isinstance(original_hp, ParentHook) else ParentHook(original_hp, self) From dd0c91de879599de0755c8acd4bf9dae87d72e61 Mon Sep 17 00:00:00 2001 From: Joachim van der Herten Date: Tue, 22 Aug 2017 00:50:46 +0200 Subject: [PATCH 3/4] Increase test coverage on ParentHook --- testing/test_modelwrapper.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/testing/test_modelwrapper.py b/testing/test_modelwrapper.py index af1b7b1..413f532 100644 --- a/testing/test_modelwrapper.py +++ b/testing/test_modelwrapper.py @@ -146,4 +146,8 @@ def test_parent_hook(self): self.assertTrue(m._needs_recompile) self.assertFalse(hasattr(p.predictor, '_predict_f_AF_storage')) + self.assertEqual(m.highest_parent.get_free_state, p.get_free_state) + m.highest_parent._needs_setup = True + self.assertTrue(hasattr(p, '_needs_setup')) + self.assertTrue(p._needs_setup) From 7e4c04bd678379c04f322478c07030dc1d50284a Mon Sep 17 00:00:00 2001 From: Joachim van der Herten Date: Tue, 22 Aug 2017 15:00:21 +0200 Subject: [PATCH 4/4] Removed non-required parenthesis --- GPflowOpt/scaling.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GPflowOpt/scaling.py b/GPflowOpt/scaling.py index cad4179..b2a8824 100644 --- a/GPflowOpt/scaling.py +++ b/GPflowOpt/scaling.py @@ -90,7 +90,7 @@ def input_transform(self, t): :param t: :class:`.DataTransform` object: the new input transform. """ - assert (isinstance(t, DataTransform)) + assert isinstance(t, DataTransform) X = self.X.value self._input_transform.assign(t) self.X = X