feat: rand(d, n) batch sampler with a guaranteed scoring round trip (#276)#296
Open
seabbs-bot wants to merge 1 commit into
Open
feat: rand(d, n) batch sampler with a guaranteed scoring round trip (#276)#296seabbs-bot wants to merge 1 commit into
seabbs-bot wants to merge 1 commit into
Conversation
…276) `rand(d, n)` draws n labelled records from a Sequential/Parallel composer into a Tables.jl column table that scores straight back through `logpdf(d, table)`, closing the simulate/score round trip for the multi-child composers. Previously `rand(tree, n)` stack-overflowed (no batch method) and a Vector of value-name records fed to `logpdf` collided with the flat single-record method. The batch commits to the VALUE-name column layout the single-record `rand(d)`/`logpdf(d, ::NamedTuple)` already use, so the round trip holds by construction. `logpdf(d, ::NamedTuple)` gains a Tables.istable branch (mirroring Choose) to score a column table per row; a vector-of-records `logpdf` dispatches on the concrete Sequential/Parallel types (not a Union) so it stays strictly more specific than the flat single-record method and never shadows it. The event-name view stays reachable through event_names/event_tree; a record's absolute positions through event_times — no third conversion path added. Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
Contributor
|
📖 Documentation preview is ready! View the docs for this PR at: https://EpiAware.github.io/ComposedDistributions.jl/previews/PR296/ This preview will be updated automatically when you push new commits. |
Contributor
Try this Pull Request!Option 1: Julia Package ManagerOpen Julia and type: import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/EpiAware/ComposedDistributions.jl", rev="feat/276-batch-records")
using ComposedDistributionsOption 2: Local CheckoutIf you have the repo locally: git checkout feat/276-batch-records
julia --project=. -e "using Pkg; Pkg.instantiate()" |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Contributor
Benchmark comparison vs baseMinimum time per call. Buckets are PR time as a % of base, so lower is faster (🟢 faster, ⚪ within 5%, 🔴 slower). Counts of benchmarks per bucket:
Evaluation — 17 benchmarks (by time change)
AD gradients — 28 benchmarks (by time change)
|
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
1 participant
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.
Closes #276.
What
rand(d, n)on aSequential/Parallelcomposer now returns a Tables.jlcolumn table of
nlabelled records that scores straight back throughlogpdf(d, table), closing the simulate/score round trip. Before this,rand(tree, n)stack-overflowed (no batch method) and aVectorof records fedto
logpdfcollided with the flat single-record method.DECISION for review: batch column layout (Sam can veto cheaply)
The issue says the batch "commits to ONE of the two existing layouts." The two
candidates:
onset_fever,path_step_a, …) — one column per emittedleaf value, the layout
rand(d)andlogpdf(d, ::NamedTuple)already use forSequential/Parallel.
origin, targets…) — origin slot + one per edge, the layoutthe one_of nodes and the fitting/record path use.
I chose value names. Reasons: (1)
logpdf(tree, rand(tree, n))round-tripsby construction, which is the property users hit first; (2) standardising on
event-names would change Sequential/Parallel's released single-record
convention — breakage nobody asked for; (3) per-composer batch layouts differing
(one_of stays event-name keyed) simply mirrors their single-record conventions
already differing, which is the self-consistent place to be.
Event-name escape hatch — no third path added. The event-name view stays
reachable through the existing
event_names/event_tree(schema) and, for arecord's absolute positions,
event_times/event_increments(the transformsjust merged in #290). I deliberately did not add a new value→event record
converter: a general per-record key remap isn't 1:1 for arity-mismatched trees,
so it would be an ill-defined third path. Happy to add a scoped one if you want
it with defined semantics.
Dispatch safety
The vector-of-records
logpdfis defined on the concreteSequentialandParalleltypes (not aUnion) with anAbstractVector{<:NamedTuple}elementtype, so it is strictly more specific than the flat
logpdf(d, ::AbstractVector)method and cannot shadow it for scalar-valued trees. Aqua's ambiguity check
passes. There is a regression test for that exact collision, both directions
(a flat
[1.0, 2.0]stays flat; aVectorof records is a batch).Tests
test/composers/batch_records.jl(4@testitems): column-table round trip forSequential/Parallel/mixed, a nested-resolve tree, value-name columns + rng
reproducibility, and the shadowing regression.
task test-fastandtask test-qualitypass.Files
New
src/composers/batch_records.jl; a 3-lineTables.istablebranch added tologpdf(d, ::NamedTuple)innamed_outputs.jl(mirrors Choose's table path).This was opened by a bot. Please ping @seabbs for any questions.