-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Conjugacy safety and flexibility by adding more validation #444
Improve Conjugacy safety and flexibility by adding more validation #444
Conversation
Separated conjugacy pairs into different objects. Added first attempt at verifying whether conjugacy is allowed.
@amal-ghamdi I have now implemented the conjugacy safety framework which is very strongly based on what Jasper did. Could you do a technical review? Also @jeverink, it would be great if you want to have a look. In particular I was wondering if you have a case that we use this |
Thanks for the work @nabriis , here is my take on In terms of conditioning (I believe) we should be able to handle I can quickly come up with:
The current version of this PR can only handle cases 1 and 4, by checking using If I recall correctly, replacing the identity check with a linearity check, i.e., I suspect that a final generalization to case 5 and cases 3 and 6 (if wanted) should not be too much additional work then. (Please fix the |
@nabriis, I currently think that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: amal-ghamdi <amal.m.alghamdi@gmail.com>
Thanks for the input. I agree. I added #453 |
…locations in likelihood
Hi @amal-ghamdi and @jakobsj. As discussed this PR is now ready for you both :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @nabriis and @jeverink for joint work for adding safety checks on the conjugate sampler. The solution with adding conjugate pair as a base class and specific concrete pair classes is very neat. I see also comprehensive tests are added, and a demonstration of the check in the demo, very nice.
My only request here, is for some general documentation to be added, as especially now with the introduction of the pair class, there are many concepts in play. Specifically, I think it should be explained somewhere:
- what conjugacy is,
- define what a conjugate pair is,
- in ConjugatePair.sample() is is mentioned "sample from the conjugate distribution" which in singular is unclear when we need a pair of conjugate distributions, what is meant by "THE conjugate distribution"
- "conjugate parameter"
I don't know where's the best place to put this, docstring of the ConjugatePair ABC perhaps since general, though may not be so visible there. Maybe in docstring of ConjugateNew sampler?
There is a comment in the docstring of ConjugateNew that the sampler does NOT check correctness, can this comment be removed now this PR adds check for correctness?
I updated the description now. Hope it helps. I think we could write a good tutorial later on conjugacy, but perhaps this is sufficient for this PR. |
Co-authored-by: Jakob Sauer Jørgensen <jakobsj@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a bit of documentation. Agree this will suffices to clarify the different concepts and connections for now and have added an issue for a tutorial as suggested: #458
Closes #345
Closes #174
Best reviewed in "side-by-side" mode.
ToDo