Move VarNamedTuple from DynamicPPL into AbstractPPL#178
Open
sunxd3 wants to merge 3 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
- Coverage 92.14% 89.67% -2.47%
==========================================
Files 17 26 +9
Lines 2100 3090 +990
==========================================
+ Hits 1935 2771 +836
- Misses 165 319 +154 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
AbstractPPL.jl documentation for PR #178 is available at: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
VarNamedTupleis a lossless,VarName-keyed parameter container that currently lives in DynamicPPL. Other frontends want to use it without depending on DynamicPPL's compiler stack: JuliaBUGS needs it to return structured samples (asAbstractMCMC.ParamsWithStatswithVarNamedTupleparameters — companion PRs: TuringLang/AbstractMCMC.jl#212 and TuringLang/JuliaBUGS.jl#517), and FlexiChains' DynamicPPL extension already carries a TODO anticipating exactly this move.VarNamedTupleis keyed byVarNameand dispatches on AbstractPPL's optic types throughout, so it cannot live in a package below AbstractPPL; a standalone package would add a registry hop without decoupling anything. AbstractPPL is its natural home, and there is precedent — theoftype system moved here from JuliaBUGS in 0.15.3.What this PR does
src/varnamedtuple.jlandsrc/varnamedtuple/(VNT core,PartialArray/GrowableArraymachinery, mapping/densification/skeleton helpers,@vnt, show methods) from DynamicPPL into AbstractPPL, with the full test suite carried over (test/varnamedtuple.jl, ~2,200 lines; export parity with DynamicPPL's copy verified).VarNamedTuple,@vnt,map_pairs!!,map_values!!,apply!!,densify!!,skeleton,subset,NoTemplate,SkipTemplate.templated_setindex!!and theVarNamedTuplesmodule arepublicbut not exported, matching DynamicPPL.AbstractPPLComponentArraysExt).Distribution-awarehasvalue/getvaluemethods (including theLKJCholeskyreconstruction path) live in the existing Distributions package extension instead of being hard-wired, so the core gains no new dependency. Behavior is equivalent whenever Distributions is loaded.docs/src/varnamedtuple.md) and aHISTORY.mdentry.Intentional deltas from DynamicPPL's copy
Beyond namespacing, there are two deliberate differences — noted here so the follow-up DynamicPPL migration doesn't rediscover them as regressions:
Base.similar(ga::GrowableArray, sz::Tuple)is narrowed tosz::Tuple{Vararg{Union{Integer,Base.OneTo}}}(DynamicPPL's copy accepts anyTuple).hasvalue/getvaluemethods moved from hard-wired code into the Distributions package extension (see above).A stale docstring advertising a nonexistent
allow_new=Val(true)keyword was also fixed.What might break
"0.15, 0.16"with zero code changes.using DynamicPPL, AbstractPPL(both unqualified) will see ambiguous bindings on use of those names once DynamicPPL's compat admits 0.16, until DynamicPPL migrates to re-export AbstractPPL's implementation. DynamicPPL itself is unaffected (its explicit imports shadow the implicit ones), and Turing imports AbstractPPL qualified, so it is unaffected too.subsetbecomes a generic function here. DynamicPPL exports its ownsubset; its migration should extendAbstractPPL.subsetrather than define a parallel function.Suggested sequencing
VarNamedTuplefor structured sample output.subsetintoAbstractPPL.subset, and move the remaining VNT design docs over. Until that lands, DynamicPPL'ssrc/varnamedtuple/is best treated as frozen, with fixes landing here first and back-ported as needed.