Closed devmotion closed 1 year ago
Base: 95.51% // Head: 97.33% // Increases project coverage by +1.81%
:tada:
Coverage data is based on head (
f36b5d0
) compared to base (2d31f09
). Patch coverage: 100.00% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I wonder if we could/should add some tests in AbstractMCMC, in addition to the existing downstream tests.
Though I agree tests would be ideal, if you're busy atm I'm happy to merge as is given that downstream tests pass.
Oh actually one question @devmotion : do we need LogDensityModel
after this PR?
I would say, yes, it's still useful since it allows downstream packages to clearly state the AbstractModel
-type(s) that they want to support with their sampler(s). Additionally, a dedicated model type for log densities makes it easier to define conversion from/to other model types.
I'll see if I find time to add some tests tomorrow. Otherwise I'll merge on Monday, if there are no objections.
I added tests but let's merge https://github.com/TuringLang/AbstractMCMC.jl/pull/114 first (I noticed CompatHelper was disabled due to inactivity).
This PR adds implementations of
sample
for generic log density functions. It is assumed that models in thesample
calls that are not subtypes ofAbstractModel
are log density functions. They are supposed to support the LogDensityProblems interface (this is checked) and wrapped in aLogDensityModel
automatically, which is then used instead of the log density in thesample
call.The motivation is that in downstream PRs to me it seemed a bit inconvenient that users have to wrap their log density functions manually. By adding aupport for automatic wrapping to AbatractMCMC instead of downstream packages, all downstream packages can benefit from it without additional code in downstream packages (they only have to support LogDensityModel).
A potential problem could be method ambiguity issues. However, since AbstractSampler is owned by AbstractMCMC this would only be partly AbstractMCMC's fault and I assume it won't happen in practice (let's wait for the downstream tests).