Closed sunxd3 closed 1 month ago
An alternative approach I can envision is fully adopting LogDensityFunction
in Turing through the ExternalSampler
interface, but this might requires much more serious work encapsulating the InferenceAlgorithm
s. (Do we have plan to do this during the coming months?)
Also for SimpleVarInfo
, I opted in OrderedDict
. To use NamedTuple
, it requires to predetermine the variable names (ref https://github.com/TuringLang/DynamicPPL.jl/blob/48487cc471543030699e3a39776dd939756d0165/src/simple_varinfo.jl#L62-L65), in principle can be done, but need a bit of work.
This also means, when SimpleVarInfo
through the changes proposed in this PR may be less performant.
@torfjelde @yebai @devmotion does this PR make sense? If this is desirable, then I'll extend tests in https://github.com/TuringLang/DynamicPPL.jl/blob/master/test/sampler.jl.
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
src/simple_varinfo.jl | 0 | 2 | 0.0% | ||
src/sampler.jl | 9 | 15 | 60.0% | ||
<!-- | Total: | 9 | 17 | 52.94% | --> |
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
src/sampler.jl | 2 | 84.75% | ||
<!-- | Total: | 2 | --> |
Totals | |
---|---|
Change from base Build 9062140401: | -1.0% |
Covered Lines: | 2775 |
Relevant Lines: | 3582 |
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
src/simple_varinfo.jl | 0 | 1 | 0.0% | ||
src/sampler.jl | 17 | 19 | 89.47% | ||
<!-- | Total: | 17 | 20 | 85.0% | --> |
Totals | |
---|---|
Change from base Build 9099752668: | 0.03% |
Covered Lines: | 2660 |
Relevant Lines: | 3428 |
cc @willtebbutt
@willtebbutt can you help review this PR?
Having a look @sunxd3 :+1:
@torfjelde is my understanding of SimpleVarInfo with NamedTuple correct at https://github.com/TuringLang/DynamicPPL.jl/pull/606#issuecomment-2111867290?
we need to make it possible to use the NamedTuple version
I agree.
Other than performance, I thought SimpleVarInfo is also less error-prone for AD (correct me if wrong), but I am unsure if AbstractDict version of SimpleVarInfo works better than VarInfo.
s my understanding of SimpleVarInfo with NamedTuple correct at
Yep! If you don't "seed" SimpleVarInfo{<:NamedTuple}
with the correct values, then it will only be sensible for models containing only varnames of the form VarName{sym,typeof(identity)}
. Now that we have "debugging" capabilities, we could "check" this, but that would ofc not be 100% reliable.
Other than performance, I thought SimpleVarInfo is also less error-prone for AD (correct me if wrong)
Not really. Or rather, I don't think VarInfo
is particuarly error-prone for AD either. SimpleVarInfo{<:NamedTuple}
might be for some of the more recent AD backends, e.g. Tapir.jl and Enzyme.jl, but only because it improves type-stability (which is not hte case of SimpleVarInfo{<:Dict}
)
Let me look into it and see if I can make NamedTuple variant of SimpleVarInfo work, or at least a clear TODOs
Let me look into it and see if I can make NamedTuple variant of SimpleVarInfo work, or at least a clear TODOs
Lovely:) And I don't mean to be negative about this btw. I'm just not a huge fan of adding additional kwargs, etc. unless there's a clear reason to use them (because otherwise nobody will ever use them until they suddenly are riddled with uncaught bugs because nobody uses it). So if we're going to add an additional kwarg to use SimpleVarInfo
, we should simultaneously prove that it has utility, i.e.:
SimpleVarInfo{<:NamedTuple}
, because that definitively has utility.SimpleVarInfo
is used, and nowhere do we implicitly convert to VarInfo
before we merge this into DynamicPPL.jl.Sounds good. I started this as a quick and dirty prototype, it definitely needs more work, until we can justify complicating the interface.
This is no longer necessary since Tapir now works with all tracing types.
Partially address https://github.com/TuringLang/Turing.jl/issues/2213
An example
Work with Turing
This should work with Turing's
Inference
pipeline with almost no modification, the only change is https://github.com/TuringLang/Turing.jl/blob/56f64ec5909cec4a5ded4e28555c2b289020bbe1/src/mcmc/Inference.jl#L319 toThis allows
bundle_samples
to use this function.Then