Internals to NamedTuple#54
Conversation
|
@penelopeysm can i get a review here? |
There was a problem hiding this comment.
Pull request overview
Updates how sampler internals are represented when constructing FlexiChains.FlexiChain outputs: internals are now passed as a NamedTuple (instead of being appended into a single numeric matrix), preserving natural element types like Bool and aligning numeric storage with the sampler’s working precision.
Changes:
- Switch internals collection (
logp,accepted,step_size,is_warmup) from stackedMatrix{Float64}to aNamedTupleof typed vectors. - Make chain numeric element types follow sampler precision (e.g.,
Float32sampler yieldsFloat32chain columns). - Update tests to assert
Bool-typed internals and addOrderedCollectionsdependency for ordered data storage.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
test/test-Adaptive-MALA.jl |
Updates assertions to expect Bool for accepted/is_warmup. |
test/test-AbstractMCMC-Interface.jl |
Adds coverage for sampler precision propagation and Bool internals. |
src/ParallelMCMC.jl |
Imports OrderedDict to support ordered per-key data construction. |
src/interface.jl |
Core refactor: build FlexiChain from vals + internals::NamedTuple, preserve types, and follow sampler precision. |
Project.toml |
Adds OrderedCollections to [deps] and [compat]. |
ext/DynamicPPLExt.jl |
Updates extension method to consume internals::NamedTuple and reconstruct stats per-sample. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "param_names consume $(offset - 1) columns, but there are $D parameter columns" | ||
| ), | ||
| ) | ||
| return FlexiChains.FlexiChain{TKey}(arr, all_names) |
There was a problem hiding this comment.
Mmmh this kind of sucks but I see why you had to do it.
Are the parameters always guaranteed to be of the same type (f32 or f64 or whatever)? If so, it might be easier to construct two separate chains, one for the params (using the existing from-Array constructor) and one for the extras, and then merge them.
There was a problem hiding this comment.
Yes that looks great, you can also move the isempty check on L654 to before the OrderedDict construction :)
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (82.22%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
+ Coverage 82.05% 82.22% +0.16%
==========================================
Files 8 8
Lines 1159 1170 +11
==========================================
+ Hits 951 962 +11
Misses 208 208 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Changes Internals to a
NamedTupleas recommended in #44 before being passed to aFlexiChain.Resolves #49