Lazy setup #68
Lazy setup #68
Conversation
Codecov Report
@@ Coverage Diff @@
## master #68 +/- ##
==========================================
+ Coverage 99.79% 99.79% +<.01%
==========================================
Files 17 17
Lines 960 994 +34
==========================================
+ Hits 958 992 +34
Misses 2 2
Continue to review full report at Codecov.
|
|
Solved the incompatibility, think I covered all aspects now. |
| # 2 - setup | ||
| # Avoid infinite loops, caused by setup() somehow invoking the evaluate on another acquisition | ||
| # e.g. through feasible_data_index. | ||
| hp._needs_setup = False |
There was a problem hiding this comment.
this can be directly at the beginning of the if-clause for clarity. make sense imo
| assert (optimize_restarts >= 0) | ||
| self.optimize_restarts = optimize_restarts | ||
| self._optimize_models() | ||
| self._needs_setup = True |
There was a problem hiding this comment.
shouldn't this be highest_parent._needs_setup?
There was a problem hiding this comment.
this happens in the constructor, under this setting highest_parent == self because its not in a hierarchy yet.
GPflowOpt/acquisition/acquisition.py
Outdated
| for oper in self.operands: | ||
| oper.setup() | ||
| if oper.constraint_indices().size == 0: | ||
| oper._setup_objectives() if isinstance(oper, AcquisitionAggregation) else oper.setup() |
There was a problem hiding this comment.
If there are no constraint idx at this level, there wont be any lower in the tree?
Perhaps it is more proper to have setup() defined in Acquisition (like below: calling constraints then objectives setup) and _setup_objectives and _setup_constraints defined as empty by default, to be overrided.
There was a problem hiding this comment.
I think that would make the interface for implementing acquisition functions loaded with complexity which is only relevant to the internal functioning.
GPflowOpt/acquisition/acquisition.py
Outdated
| if oper.constraint_indices().size == 0: | ||
| oper._setup_objectives() if isinstance(oper, AcquisitionAggregation) else oper.setup() | ||
|
|
||
| def setup(self): |
There was a problem hiding this comment.
so this could be in Acquisition? not overridable
There was a problem hiding this comment.
Hmm, of course with separate setups no information can be easily changed between setup_constraints and setup_objectives.
There was a problem hiding this comment.
Ok, we discussed this offline and I'm getting rid of the isinstance() calls
GPflowOpt/acquisition/hvpoi.py
Outdated
| num_objectives = self.data[1].shape[1] | ||
| assert num_objectives > 1 | ||
|
|
||
| # Keep pareto empty for now - its updated in setup() |
Added missing NotImplementedError for assign method
This modification to acquisition automatically delays _optimize_models and setup() until it is required for proper functionality. A decorator is provided to mark methods which should trigger the setup.
Not everything I was planning for this has already been added, but it offers some insight in what is going on. I was in doubt between a single _needs_setup to be looked after in the highest parent, or doing this on the level of each acquisition. The latter could be used to be slightly more efficient I think, but on my local copy I noticed it increases the code complexity a lot as nearly all methods were changing the status of the _needs_setup flags in the tree. The current setup is slightly simple.
The setup decorator is a class although I was aiming for a function. Unfortunately, there is some incompatibility with the AutoFlow decorator there, I'm still investigating.