feature/laplace dynamic delta - #566
Conversation
| else: | ||
| delta = self.delta | ||
|
|
||
| projection, status = factor_approx.project( |
There was a problem hiding this comment.
This projection step does not occur anymore. @matthewghgriffiths is this correct?
| logger = logger.debug | ||
|
|
||
| def __init__(self, initial_values=None, deltas=None, inplace=False, delta=1): | ||
| def __init__(self, initial_values=None, deltas=None, inplace=False, delta=1, dynamic_delta=False): |
There was a problem hiding this comment.
Currently the default optimiser would need to be passed with dynamic_delta=True explicitly. I'm tempted to make the default dynamic_delta=True
There was a problem hiding this comment.
Once I test this feature set properly I suspect we'll end up making it True by default.
There was a problem hiding this comment.
Yeah makes sense. I also need to look at incorporating it into the Laplace optimiser
There was a problem hiding this comment.
Oh wait that's what this PR does lol
| self.deltas = deltas or {} | ||
| self.dynamic_delta = dynamic_delta | ||
|
|
||
| def update_model_approx( |
There was a problem hiding this comment.
Suggest deleting this method and merging functionality into EPOptimiser class
| return new_approx, status | ||
|
|
||
| @abstractmethod | ||
| def optimise( |
There was a problem hiding this comment.
This method should have signature,
def optimise(
self, factorApprox: FactorApproximation
) -> Tuple[FactorApproximation, Status]:
matthewghgriffiths
left a comment
matthewghgriffiths
left a comment
There was a problem hiding this comment.
Finishing review
|
|
3 participants
Better unifies the optimize function between Laplace and other optimisers.
This allows the delta computed from the number of shared factors for a given variable to be applied in the case of Laplace. It also illustrated the in the case of classical autofit optimisers a projection was applied to a factor then to the model; in the case of Laplace it only seems that projection is applied to the model. Classical optimisers now implement the same behaviour as Laplace.