A fairly large reworking to allow entire method objects to be returned #434
Conversation
Matches what we do in the variational case, and we don't use this code much now anyway
Fixes tskit-dev#73 . Also adds some debugging comments to variational_gamma
|
Once this is in, we can start saving measures of EP convergence in the ExpectationPropagationModel class. |
Alos raise an error when accessing node_posteriors for maximization
CHANGELOG.md
Outdated
| **Breaking changes** | ||
|
|
||
| - The `return_posteriors` argument has been removed and replaced with `return_model`. | ||
| An instance of one of two previously internal classes, `ExpectationPropagationModel` |
There was a problem hiding this comment.
This is great! But I have one very pedantic request-- can we not add "Model" to these class names, as they're not models but algorithms to fit models. We could prepend "Algo" or "Fit" instead, or leave as-is.
There was a problem hiding this comment.
Yeah, I was also a little uncomfortable about "model", but I want to be able to say
ts, something = tsdate.date(ts, mu, return_something=True)Have you got a better suggestion for "something". I guess as you say, "fit" might work actually?
There was a problem hiding this comment.
"return_fit" is fine, I think. I'd prefer to keep the class name as "ExpectationPropagation", no need to add a suffix.
There was a problem hiding this comment.
Good call. I completely see your point: "fit" is more appropriate and accurate than "model" (as well as it being shorter to type), so thanks for that suggestion.
The equivalent to the "ExpectationPropagation" class is now called "InsideOutside", but I'm not so happy with that name, partly because that fit object is returned from both the inside-outside and the outside-maximisation methods. Do you by any chance have a better suggestion for a name describing that non-variational approach?
There was a problem hiding this comment.
(Of course, it could be that it's possible to implement EP with the discrete-time method too, but let's cross that bridge later, if we ever come to it)
There was a problem hiding this comment.
Do you by any chance have a better suggestion for a name describing that non-variational approach?
BeliefPropagation?
|
Thanks for the useful feedback @nspope : merging so we don't get too out-of-sync. |
This replaces
return_posterior, and fixes #410.It also rationalises some file naming, and moves discrete methods to their own files.