Sprint15 add multiple likelihood posterior #151
Conversation
|
Hi @amal-ghamdi, I finished adding the new MultipleLikelihoodPosterior class. I also added a HowTo for defining Posteriors as I felt this was relevant now with this class. |
There was a problem hiding this comment.
Thank you @nabriis. The PR is really great! I have only few comments.
There was a problem hiding this comment.
Thank you @nabriis for your updates and improving the robustness of this feature. I just added a minor documentation comment and I have one last thought about the class name. SingleVariablePosterior gives a sense that it is more of a special case of the posterior, not a generalization. But I leave it to you and @jakobsj if you think we should update the name or keep it for now.
Looks great to me!
There was a problem hiding this comment.
Thank you for adding this great new feature. After discussing it (again) today, I am happy to see it merged, with a few minor changes and as discussed renaming back to MultipleLikelihoodPosterior.
We discussed this PR as step toward a new zoo of Posterior classes with specific purpose and behaviors, probably organized into a subclass structure with a generic (Joint?)Posterior at the top, from which branches of dedicated subclasses could be formed, such MultipleLikelihoodPosterior and from that perhaps the existing Posterior as a special case of single likelihood. This would likely involve a JointPosterior and we should decide on systematic naming conventions. Please before merging this PR could you create one or more issues (likely not labelled ready) to capture this idea.
|
Hi @jakobsj thanks for the comments! |
Closes #123.
I left the MultipleLikelihoodPosterior in the joint distribution file because it uses both Distribution and JointDistribution, which makes theimports circular if not within the same file.
Edit: This has turned into a SingleVariablePosterior class. At the moment our joint distribution does not fully support multiple distributions for the same variable. This is because we enforce that each density must have a unique name. If we update this, the SingleVariablePosterior can work with multiple priors for the same parameter.