The 0.2.0 leaf-and-node contract, as one diff against main - #381
Open
seabbs-bot wants to merge 44 commits into
Open
The 0.2.0 leaf-and-node contract, as one diff against main#381seabbs-bot wants to merge 44 commits into
seabbs-bot wants to merge 44 commits into
Conversation
`_update_leaf` now calls `rebuild_leaf(leaf, vals)`, an ordinary method with no constructor-identity contract, instead of building through `leaf_ctor` directly. `tie` groups leaves by the new `leaf_signature(leaf)` hook, which keeps the egal-stability requirement that only tie identity actually needs. A leaf whose free parameters are not its native constructor arguments can now override `rebuild_leaf` (an ordinary method) rather than being forced into a callable-struct `leaf_ctor` just to satisfy `tie`'s equality check.
Both hooks' defaults previously dispatched on the outer wrapped leaf directly, so an override on an inner leaf (e.g. one wrapped in truncated/uncertain) was never consulted: rebuild_leaf re-entered leaf_ctor's own peeling instead of the override, and leaf_signature could mis-group structurally distinct wrapped leaves under tie. Peel one free_leaf layer at a time before dispatching, mirroring how leaf_ctor already peels, so an inner-leaf override is honoured under any wrapper stack. leaf_signature keeps the parameter-names half keyed on the outer leaf so a wrapper's own extra_leaf_params names are not dropped. Add a testitem covering both hooks under truncated/uncertain wrappers, including a tie-rejection case that fails without the fix.
Introduce composed_to_table(d), the full node/attribute/param inventory of a composed tree, alongside the existing params_table(d). Both are produced by one pre-order walk threading a sink object (_ParamSink or _FullSink) instead of five positional column vectors, so params_table stays the exact role == :param projection with no extra traversal or allocation on its existing AD-adjacent call sites (e.g. centred_pool_rows). Every composer node and leaf (wrapper) layer gets a :node row and any node_attributes :attribute rows; a leaf's wrapper stack (Truncated, Censored, Shared, Varying, Uncertain) is listed via the new leaf_layers hook, one row per layer at the leaf's real path, emitted before the shared-tag dedup so every shared occurrence is visible structurally even though its :param rows are still inventoried once under the tag. A Resolve's own branch_probs rows stay emitted after its children's rows, matching the codec's type-level walk order. node_kind, node_children (renamed from _node_children, aliased for source compatibility), node_attributes and leaf_layers are the new public node-emission hooks a downstream node/leaf-wrapper type overrides. Golden projection-parity, row-invariant and structural-recovery tests cover Sequential/Parallel/Resolve/Compete/Choose, shared and pooled leaves, Truncated/Censored/Varying/Uncertain wrappers, a Convolved composite, and a third-party leaf wrapper that only extends free_leaf/rewrap_leaf (an opaque node row, by design).
A composed distribution is now a Tables.jl source in its own right: Tables.columns/columnnames/getcolumn/schema/rows all forward to its full composed_to_table, so DataFrame(tree) yields the full table (DataFrame(params_table(tree)) stays the parameter-only one). No Base.getproperty override, so field access on every node keeps working. Because a composed distribution is now Tables.istable, update(a, b) with b a tree would otherwise silently reach update's table arm and bulk-write b's rows into a. Add an explicit update(::AbstractComposedDistribution, ::AbstractComposedDistribution) guard that throws, naming composed_to_table/params_table as the explicit way to copy another tree's rows. update(d, table) now filters a role-carrying table (composed_to_table or a DataFrame of one) to its role == :param rows first, so passing a tree or its full table straight to update only ever writes parameters; a table with no role column (the existing params_table shape) is unaffected.
Mention composed_to_table alongside params_table in the introspection contract section of the developer interface-contracts page, and add an Unreleased NEWS.md entry summarising the new full table, the tree's Tables.jl forwarding, and role-aware update. Every new public docstring (composed_to_table, node_kind, node_children, node_attributes, leaf_layers) already carries # Arguments and a runnable @example, verified against a full docs build.
`_walk_rows!`'s leaf branch built and iterated `leaf_layers(leaf)` unconditionally, so `params_table`'s AD-hot path paid for the full layer walk even though `_ParamSink`'s node/attribute pushes are no-ops. Add `_emit_layers!`, dispatched on the sink: a no-op for `_ParamSink`, the layer loop for `_FullSink`, so the params-only path never touches `leaf_layers` at all.
…e table surface BREAKING CHANGE: params_table(d) is removed. composed_to_table(d) is now the only table-reading verb: it returns the full node/attribute/parameter inventory, and the parameter-only view is a filter over it (`filter(row -> row.role == :param, Tables.rows(composed_to_table(d)))`). This folds into the unregistered 0.2.0 breaking window rather than a deprecation cycle. - build_priors and update now both accept a composed_to_table-shaped table directly, filtering to :param rows internally the same way, so build_priors(composed_to_table(tree)) and update(tree, composed_to_table(tree)) work without hand-filtering. - centred_pool_rows (Pool.jl) and required_parameters (varying.jl) migrate to the _ParamSink walk directly, the same zero-extra-work path params_table used internally, so neither regresses to building the full table on their AD-adjacent call sites. - ParamsTable's show header drops the now-dead params_table/composed_to_table branch. - Tests recast the golden parity fixture as an explicit role == :param filter over composed_to_table, verified against an internal _param_rows helper that still runs the parameter-only walk independently (proving the filter reproduces the historical params_table output). Other tests move to composed_to_table directly where row count is not significant, or to _param_rows where a test's assertions depend on the historical five-column shape or exact row count. - Docs and tutorials teach composed_to_table plus the role filter as the parameter-view idiom.
guards and tests Reviewer follow-ups on the #227 params_table removal: - extend the leaf hot-path testitem to exercise required_parameters and centred_pool_rows directly, not just _param_rows - rename the composed_to_table wrapper type from ParamsTable to ComposedTable (breaking, same unregistered 0.2.0 window) - add the missing # Arguments section to composed_to_table's docstring - pin and document the empty-path/root-row convention that required_parameters and centred_pool_rows fall back to for a bare leaf - give build_priors the same up-front Tables.istable/edge/param guard update already has, so a DI-shaped table is refused loudly - dedupe the restated row-structure sentence in NEWS.md and fix an unrewrapped line
Add a single-layer peel hook inner_dist(leaf) whose base identity returns the
leaf itself. Every read-through leaf-wrapper hook (free_leaf, uncertain_specs,
extra_leaf_params, shared_tag) now recurses through inner_dist, so a wrapper
type registers ONE method returning its inner distribution instead of one
redundant forwarding method per hook. Register inner_dist for Truncated,
Shared, Uncertain and Varying, and drop their now-redundant explicit forwards.
Rather than the 16 one-line peel methods a wrapper currently needs in the
ModifiedDistributions extension, it now needs 4 inner_dist registrations:
inner_dist(d::Affine) = get_dist(d)
inner_dist(d::Weighted) = get_dist(d)
inner_dist(d::Transformed) = get_dist(d)
inner_dist(d::Modified) = get_dist(d)
The generic peel recurses until the base identity (inner === leaf) stops it,
which is the terminal for a plain leaf. Rebuild hooks (rewrap_leaf,
set_extra_leaf_params, instantiate) and special-cased hooks (has_varying,
Shared's shared_tag, Uncertain's uncertain_specs) stay explicit where their
semantics are not a pure forward. Backward-compatible: existing explicit
wrapper methods still win by dispatch specificity over the new generics.
Closes the leaf-wrapper boilerplate half of the leaf-protocol simplification.
#352) _leaf_flatten_grouped/_leaf_flatten_walk_grouped (Pool.jl) call leaf_param_names(leaf) at runtime and recurse over the result as a plain Tuple. Every built-in leaf's param_names is a literal tuple, so this happens to constant-fold and @inferred passes today -- which is why the existing pooling and S2 codec-parity tests never caught the gap. Add a leaf whose param_names comes from a Ref read (not effect-free, so it can't fold even though its TYPE still infers concretely) inside a pooled tree. @inferred flatten fails: "return type Vector{Float64} does not match inferred return type Any". Red, on purpose -- the fix lands next.
… Tuple (#352) _leaf_flatten_grouped/_leaf_flatten_walk_grouped derived their walk order by calling leaf_param_names(leaf) at runtime and recursing over the result as a plain Tuple, hoping the whole call constant-folded so `pname` stayed a Core.Const at each step. It does, for every built-in leaf (param_names returns a literal tuple), which is why nothing caught this. A leaf whose native names come from anything less trivially foldable (a Ref, a field behind a branch -- a realistic third-party Distributions.jl leaf) breaks the fold: `specs[pname]` can no longer resolve which field of the heterogeneous spec NamedTuple it needs, and the walk -- and `flatten` after it -- silently widens to Union/Any. Same class of bug _hyper_flatten_walk was already fixed for one level down (see its comment); this was the one remaining flatten-direction walker still on the old pattern. Fix: derive `names` from `entry`'s own NamedTuple{names} type parameter (entry's keys ARE leaf_param_names(leaf) by construction -- _leaf_entry_grouped built it that way) and dispatch the whole recursion on Val{names}/Val{speckeys}/Val{pool_names}/Val{materialize}, mirroring _leaf_extract (introspection.jl) and _hyper_flatten_walk (this file). `pname` is then part of the TYPE at every step, so it is a Core.Const regardless of whether leaf_param_names itself folds. Measured: @inferred flatten(tree, nt) on a pooled tree over the adversarial leaf now infers Vector{Float64} (was: Any). Every existing pooling/codec test still passes unchanged -- this is a pure robustness fix, not a behaviour change; the built-in-leaf flat layout, order and values are untouched (confirmed byte-for-byte on the S2 parity fixtures). unflatten's dual (_leaf_entry_grouped/_leaf_walk_grouped) has the same shape and the same theoretical exposure but is out of this item's scope (#352 names only the flatten-direction walker); flagged as a follow-up.
… test (#352) `isconcretetype(typeof(nt))` can never fail -- `typeof` of any runtime value is always concrete, whether or not the CALL that produced it inferred concretely. Replace it with `@inferred unflatten(...)`, matching the S2 parity item just above, and add `@inferred` to the `flatten` call too (it had no inference assertion at all). Both pass unchanged: MomentLeaf's param_names is a literal tuple, so this item was never exposed to the runtime-name-walk gap the Pool.jl fix addresses; this is pure test hardening, not a bug fix.
…ness Widen the closed-type composability gates (`_is_composable`, `_is_one_of_branch`, `_compose_child`, `_is_composer_node`) to dispatch structurally on the public `AbstractComposedDistribution` root instead of a `Union` of the built-in types, so a downstream node subtyping it and implementing `child_nleaves`/`child_logpdf`/`child_rand!` nests as a named child of every built-in with no registration. `has_varying`/`has_uncertain` widen the same way, riding a newly-public `node_children` accessor (renamed from the internal `_node_children`, alias kept). Fix `leaf_mean`/`leaf_var`: they used to peel through `free_leaf` for every leaf, silently reporting a `Truncated`/`Censored` leaf's untruncated moment. `Truncated`/`Distributions.Censored` now try their own `mean`/`var` first and fall back to the old approximation only when Distributions.jl has no closed form for that inner family. Add `TestUtils.test_leaf_protocol_completeness` and `TestUtils.test_sampling_consistency`, closing #277 and #278: both name the specific missing hook/assert failure on a deliberately incomplete/corrupted fixture (tested). Merge `interface-contracts.md` + `leaf-protocol.md` into one `docs/src/developer/extending.md`, fix the `@ref new-composer-node` anchor `nesting.jl` already links (pointed nowhere), and make the worked node example actually compose as a child of a built-in. This was opened by a bot. Please ping @seabbs for any questions.
The Truncated{Gamma} example ran without erroring but silently exercised
the pre-fix fallback path (Gamma has no Distributions.jl closed-form
truncated moment), so it never actually demonstrated the leaf_mean fix
the surrounding prose claims. Switch to truncated(Normal; lower), which
does have a closed form and visibly differs from the untruncated mean
(0.7979 vs 0.0), matching the regression test in test/interfaces.jl.
A leaf with vector-valued parameters, Categorical being the motivating case, silently emitted a Vector into a scalar parameter row. The table walk defines one row per scalar parameter, so there is no correct row for a probability vector. The guard existed only in Uncertain's constructor, so a fixed leaf reached the plain walk unchecked. Throw there too, naming the leaf and pointing the caller at direct parameter access. Composing and scoring such a leaf is unaffected and stays supported; only the table view is undefined for it. Salvaged from an uncommitted branch left by an earlier session. That branch also changed leaf_mean/leaf_var to stop reading through free_leaf, which is a real defect, but PR #365 fixes it better: it falls back to the free-leaf approximation when Distributions.jl has no closed form, where the unconditional version throws for Truncated{Gamma}. That half is dropped here in favour of #365.
The reworked node-emission surface derives a leaf's wrapper layers by folding `inner_dist` rather than shipping its own `leaf_layers` hook, so this branch stacks on #362.
The single-table walk shipped four public hooks. Three of them asked a downstream type for something the package can work out for itself, so only one survives. - `node_kind` is now the internal `_node_kind`. It reads the type name off the value, which is correct for every type that can appear in a tree, so there is nothing for a downstream type to supply. - `leaf_layers` is gone. A leaf's wrapper layers are the fold of the `inner_dist` peel a wrapper already defines to take part in the leaf protocol, so the internal `_leaf_layers` folds it instead of asking for a second, redundant method. `Truncated`, `Shared`, `Varying`, `Uncertain` and `Censored` lose their per-type layer methods; `Censored` gains the `inner_dist` method it was missing, which also retires its four read-through forwards. - `node_children` is left as the internal `_node_children` it was on main. Promoting it here would collide with the independent promotion in the node-surface work, which documents it as the accessor a downstream node defines for `has_varying`/`has_uncertain`. This walk consumes it under its existing internal name instead, so exactly one definition ships. `node_attributes` earns its place: a node's fixed, non-parameter structure (a `Choose`'s selector, a truncation bound, a shared tag) cannot be derived from the type or from the peel. A downstream leaf wrapper now defines two methods for the table (`inner_dist`, `node_attributes`) instead of four (`node_kind`, `leaf_layers`, `node_attributes` plus the peel), and a downstream composer node defines one (`node_attributes`) instead of three.
Two repairs the reworked branch needs to run its own gates green. - `_pool_hyper_rows!` pushed a parameter name whose type the compiler cannot pin down for a population template of non-concrete type, so the typed row push did not resolve statically. A leaf's parameter names are `Symbol`s by contract; state that at the loop, which the row push already required. - Three codec tests still called `params_table`, removed earlier on this branch, and errored on load. They assert the parameter-row projection's order, so they now run it through `_param_rows`.
The Windows Julia 1.x Test cell hung to the 60-minute job timeout. The job log's last line is the ComposedDistributionsOptimizationExt precompile, then 55 minutes of silence. That precompile is triggered by the first quantile testitem's `using Optimization, OptimizationOptimJL`, and the first thing it then does is a Nelder-Mead solve. Windows on the Julia pre release runs the same testitems, against the same Optim 2.2.1 / Optimization 5.7.0 / SciMLBase 3.44.0 resolution, in under 8 minutes. The #358 maxtime bound could not have helped. It reaches Optim's `time_limit`, which is checked between iterations, so it cannot interrupt a single evaluation. A cdf is monotone, so its inverse is a root find, not an optimisation. Resolve and Compete now bisect their own cdf over a bracket grown from the support by doubling, which terminates in a step count fixed by construction. The bracket needs no mean, so the non-finite starting point of #344 is gone too. Measured against the old solve over five node shapes at ten probabilities each, the worst |cdf(quantile(d, p)) - p| falls from 3.7e-5 to 2.2e-16, and a call is roughly nine times faster. No solver is involved any more, so the methods move into src and the Optimization / OptimizationOptimJL extension goes away. The quantile now answers with nothing but the package loaded, and a subprocess testitem pins that. A Sequential chain's quantile still delegates to ConvolvedDistributions' Convolved, whose own quantile is the same Nelder-Mead solve, so the two testitems covering that arm still load the solver stack. The same bracketed root find belongs upstream. Also make a future hang legible. TestItemRunner prints nothing until its summary, so runtests.jl now runs one test file at a time, announcing the file and its items before starting and the wall clock after. A hang names the file it is in instead of dying silently at the job timeout.
`node_attributes` is public but not exported, so an `@ref` from a page whose module context is `Main` cannot resolve it. The neighbouring leaf-wrapper section already names such hooks in plain backticks.
`param_names`/`leaf_param_names` no longer read a curated six-family table. `param_names(leaf)` now derives names generation-time, via a `@generated` two-type-argument helper keyed on `typeof(leaf)` and `typeof(params(leaf))`: the first N fieldnames of the leaf's type, transliterated (Greek letters to English), where N is params' arity. Falls back to positional `:param_1, :param_2, ...` when the fields don't line up 1:1 with params in order (too few fields, a fieldtype mismatch, or a name collision after transliteration). A wrapper reads through `inner_dist` with no method of its own. This gives real names to 57 further Distributions.jl families with zero registration (steer 1), while an explicit `param_names` override still wins by dispatch specificity for a leaf whose fields don't align with its params (steer 2). A runtime identity guard (`_check_leaf_param_alignment`) catches the one thing the type-level rule cannot see: a contract-conforming leaf whose field ORDER differs from its params order with matching field types. It runs in `Uncertain`'s inner constructor (values already in hand there) and in `TestUtils`'s conformance check over every real leaf of a tree, restricted to Real-valued slots, and never from the gradient path (`param_names`/`leaf_param_names`/codec), so it stays Mooncake-safe. It is a diagnostic only -- names never depend on values. No new public or exported name. Documented in the leaf-protocol developer page with a worked example and the alignment contract.
Update codec_gen.jl's leaf keyword/NamedTuple literals to the derived names (Gamma/Weibull shape,scale -> alpha,theta). Restore is a misnomer here -- the @inferred assertions in the "S2 layout parity" item were already present on this branch -- but add a new allocation assertion pinned to measured values: 0 B on five trees, 384 B on the pooled tree (pre-existing on the base branch, asserted against its measured value rather than 0 so it can't be mistaken for a regression introduced here). Neither @inferred assertion was weakened or removed.
Gamma/Weibull derive (:alpha, :theta) now, not the old curated (:shape, :scale); update the affected docstring examples in Pool.jl, uncertain_macro.jl and varying.jl by hand, leaving every "location-scale" (statistical term of art) and "loc, scale" (the population's own hyperparameters, a different pair of names) mention untouched.
Gamma/Weibull now derive (:alpha, :theta) instead of the curated
(:shape, :scale); rename by hand, file by file, reading every site
against the trap list before touching it:
- local variables that are not parameter names left alone (e.g. a
covariate `inc_shape`, the classifier vocabulary's own :shape/:scale
entries)
- @testitem names describing tree/moment/record "shape" (structural)
left alone; the one @testitem about a Gamma value
("at shape == 1.0", ad/scenarios.jl) left alone per its own history
- the DI-style flattened negative fixtures
(`:onset_admit_shape`/`_shape`) renamed to `_alpha` for consistency,
a conscious choice since their exact spelling doesn't affect what
they test
- ADFixtures' one real `uncertain(...; shape = ...)` kwarg site
renamed; its historical Mooncake xlogy comment left as prose
Every touched file's tests were run and pass after the rename.
composing-distributions.jl, strata-uncertainty.jl and event-skeletons.jl each construct/inspect a Gamma leaf by its now-real derived name. Ran the full docs build (SKIP_NOTEBOOKS=true julia --project=docs docs/make.jl) to confirm the Literate execution of every edited block still succeeds.
…tted Project.toml, benchmark/Project.toml, docs/Project.toml, test/Project.toml and test/ad/Project.toml were applied unstaged (per PR#359, chore/drop-convolved-sources-pin) to make this worktree's environment instantiate against ConvolvedDistributions 0.4.0. They were already staged in the index before this branch's first commit and got swept into it by mistake. Restore all five to their origin/feat/227-single-table-slice-1 content; anyone picking this branch up must reapply PR#359's patch locally and keep it uncommitted.
…ent-zero The composer-node extension contract is public (not exported) and minimal now: a downstream node implements node_children/node_rebuild/component_names (promoted from the private _node_children/_rebuild), and that alone is enough to compose, table (composed_to_table), and update. child_nleaves/ child_logpdf/child_rand! default generically off node_children for a plain "concatenating" node; a node with novel combination semantics still overrides them, as Choose does. Codec (flatten/unflatten/reconstruct/fit) support additionally needs the node's own child names and children's types as its first two type parameters, matching Sequential/Parallel/Choose/ Compete's existing shape -- documented as a deliberate choice, since a @generated function cannot call a method a downstream package defines (a measured Julia world-age hazard). composed_to_table, params, update, has_uncertain, has_varying and compose(...) nesting are now generic over any AbstractComposedDistribution subtype rather than closed to the five built-in node kinds; a node missing a required method now fails with a clear error naming the gap, rather than silently being treated as a leaf with zero estimated parameters (#374). TestUtils.test_estimation_dimension is a new conformance check (wired into both test_node_interface and test_interface) asserting flat_dimension(d) matches composed_to_table(d)'s estimated-row count, so a node that silently drops an uncertain leaf from the codec fails the harness instead of shipping green. docs/src/developer/interface-contracts.md and test/interfaces.jl are rewritten around a genuinely working Both node (subtyping AbstractComposedDistribution) that composes, tables, flattens and fits, replacing the old worked example that could not actually participate in composition. A dedicated negative-fixture testitem asserts an incomplete node fails loudly rather than silently.
) Add the missing `using ComposedDistributions, Distributions` to the MomentLeaf @example block in the leaf-protocol developer docs -- it is the first `leaf-protocol`-named example block on the page and used `ContinuousUnivariateDistribution` unqualified, so the full docs build (`task docs-fast`) failed with an UndefVarError. File #377 to track the default_prior misclassification regression for InverseGaussian.mu, SkewNormal.alpha and NormalInverseGaussian.beta (a real build_priors correctness bug), and point the NEWS.md entry and the pinning testitem at it instead of only the parent #372.
Conflict in test/composers/composers.jl: both sides appended a new testitem at the end of the file. Kept both. Repointed the tie-identity assertion at composed_to_table's role == :param projection, since #343 removed params_table.
Delete/modify conflict on docs/src/developer/interface-contracts.md and leaf-protocol.md: #343 edited both, #365 merges them into extending.md. Resolved in #365's favour (both pages deleted) and #343's wording carried across into extending.md: composed_to_table replaces params_table, and inner_dist / node_attributes join the leaf-wrapper method table and steps. Also repointed the params_table call and references #343 removed: TestUtils.test_leaf_protocol_completeness now reads composed_to_table's role == :param rows, and the stale [`params_table`](@ref) cross-references in Pool.jl and public.jl now name composed_to_table.
…#379) Conflicts and how each was resolved. docs/src/developer/interface-contracts.md (delete/modify): #365 deletes the page into extending.md; #379 rewrites it. Resolved in #365's favour and #379's wording carried across into extending.md — the three-method node contract (node_children / node_rebuild / component_names), the codec type-parameter layout convention and its world-age reasoning, the five-step recipe, and the worked `Both` node that genuinely composes, tables, flattens and fits. src/public.jl, nesting.jl, compose.jl, varying.jl, Uncertain.jl: both branches widen the same closed Unions to the abstract root. Kept #365's Multivariate-parameterised methods, which keep the generic method disjoint from the univariate one_of method rather than relying on dispatch to break a tie, and folded #379's node_rebuild into the public-surface comment. src/composers/introspection.jl: both branches promote node_children, giving two docstrings for one binding. Merged into a single docstring (#379's interface framing plus #365's has_varying/has_uncertain note), one set of methods, and the _node_children alias kept as a transitional name. src/TestUtils.jl: export list, union of both branches' additions. test/interfaces.jl: took #379's Both node, which carries names and child types in its type parameters so the codec can read its layout; #365's surrounding assertions are unchanged and still apply. Also repointed the codec_gen.jl comment at extending.md.
Conflicts and how each was resolved. docs/src/developer/interface-contracts.md and leaf-protocol.md (delete/modify): both already merged into extending.md by #365. Carried #378's wording across — a new "Parameter names" section with the type-level derivation rule, the fallback conditions, the alignment guard and the MomentLeaf worked example, plus the leaf-section pointer to it. src/composers/introspection.jl: #346's leaf_signature and #378's _check_leaf_param_alignment are separate additions in the same region. Kept both. src/TestUtils.jl: kept #378's per-leaf alignment check and #365's widened AbstractComposedDistribution table branch. NEWS.md: kept both entries. Beyond the mechanical merge: #346's FamLeaf/WrapFamLeaf testitems were written against positional param_1/param_2 names and Gamma's old shape/scale, both of which #378 renames. Repointed at the derived names (a/b for the custom leaves, alpha/theta for Gamma). No assertion dropped.
…mpat #370's guard and its test were written against params_table, which #343 removes. Both now use composed_to_table — the test asserts the same throw from the table walk, and the error message names composed_to_table's :param rows. #370's leaf_mean/leaf_var half stays dropped in favour of #365's, which falls back to the free-leaf approximation where Distributions.jl has no closed form. Also floors ConvolvedDistributions compat at 0.4.1 (now registered) in the root, test, docs, benchmark and test/ad environments.
A stacking-only failure, invisible to any single PR's CI: #365's and #379's fixtures name Gamma's parameters shape/scale, which #378 renames to alpha/theta. Four testitems and TestUtils's own default errored with 'unknown parameter :shape'. Renamed in TestUtils (test_leaf_protocol_completeness's default param, test_estimation_dimension's docstring example), test/interfaces.jl's Both and NoMethods fixtures, and the extending.md worked example. Values and assertions unchanged.
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="integration/0.2.0-contract")
using ComposedDistributionsOption 2: Local CheckoutIf you have the repo locally: git checkout integration/0.2.0-contract
julia --project=. -e "using Pkg; Pkg.instantiate()" |
Contributor
|
📖 Documentation preview is ready! View the docs for this PR at: https://EpiAware.github.io/ComposedDistributions.jl/previews/PR381/ This preview will be updated automatically when you push new commits. |
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 — 36 benchmarks (by time change)
|
…g' into integration/0.2.0-contract # Conflicts: # test/composers/codec_gen.jl
This was referenced Aug 12, 2026
Collaborator
Author
|
#363 is now folded in too (8b560a7) — it had been left out of the first integration pass, my omission. The branch still carried the untyped The merge conflicted in That closes #352 here as well. Re-gated after the merge: Assertions against The nine constituent PRs are now closed as superseded, with their branches and bodies preserved for the rationale. Open PRs are now just this one, #373 (prior removal, still on your This comment was posted by a bot. Please ping @seabbs for any questions. |
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
2 participants
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.
Eight open PRs collapsed into one branch off
main, so this reviews as a single diff instead of a stack. 63 files, +4451/−1690.Folded in, in dependency order: #376 (quantile bisection), #362 (
inner_dist), #343 (composed_to_table+ hook rework), #346 (rebuild_leaf/leaf_signature), #365 (composability + conformance harness), #379 (node interface), #378 (type-level names), #370 (vector-param guard). #359 turned out to be already merged to main, so it was a no-op.#373 (prior removal) is deliberately excluded — it still has your open
estimated::Boolquestion, and folding it in would bake in an answer you have not given.The two headline numbers
Leaf: zero methods. Verified on the integrated tree — a bare
Zilch <: ContinuousUnivariateDistributionderives(:m, :s), tables and scores with no registration of any kind.Gammaderives(:alpha, :theta).Node: three —
node_children,node_rebuild,component_names. Public, not exported.child_nleaves/child_logpdf/child_rand!now default generically offnode_children; only a node with novel semantics overrides, asChoosedoes.The correctness bug this closes
A conforming node used to get
flat_dimension == 0, so a fit on it estimated nothing and reported no error, and the harness passed it 8/8.flat_dimensionnow matches the table's estimated-row count across 14 fixtures — built-ins, shared, uncertain, truncated, censored,Convolved, centred and non-centred pools, and the zero-code leaf.test_estimation_dimensionis wired into both harnesses so this cannot regress silently.Conflicts, and what needed judgement
node_childrenwas promoted independently by three branches, each with its own docstring for the same binding — a duplicate-docstring error waiting to happen. Exactly one public definition ships now, with the docstrings merged._node_childrensurvives as a documented transitional alias becauseTestUtilsstill calls it.Where #365 and #379 both widened a closed
Unionto the abstract root, #365's{Multivariate}-parameterised methods were kept over #379's bare ones: they keep the generic method disjoint from the univariateAbstractOneOfmethod rather than relying on dispatch to break the tie.The two developer pages #365 deletes into
extending.mdare the ones #379 and #343 rewrite. Resolved in #365's favour, carrying #343's wording, #379's workedBothnode, and #378's parameter-name section across. Nothing was dropped.A stacking-only failure, of exactly the kind no single PR can catch
#365's and #379's fixtures name Gamma's parameters
shape/scale, which #378 renames toalpha/theta. Four testitems plustest_leaf_protocol_completeness's own default parameter errored withunknown parameter :shape. #346'sFamLeaffixtures hit the same class, with positionalparam_1/param_2becoming deriveda/b.Every one of those branches is green on its own. This is the second stacking-only failure in this effort — the first was a JET error that appeared only when #343 and #362 were combined.
Several stale
params_tablecross-references would also have failed the docs gate.No test or assertion was weakened
Measured against the strongest single branch:
@test1723 → 1825,@test_throws130 → 139,@inferred9 → 9.codec_gen.jl's allocation assertions ([0, 0, 0, 384, 0, 0]) are byte-identical to #378's and pass.Dependencies
ConvolvedDistributions floored at
"0.4.1, 1"across root,test/,docs/,benchmark/andtest/ad/.EpiAwareADTools sits at 0.1.2 in this diff. 0.2.0 registered while this was being built and is installable now, so the bump follows as a commit on this branch — see #380, its
logccdf_ad_safe(::Gamma)far-right-tail fix is oneCompete.jldepends on directly.One
[sources]pin remains intest/jet/Project.toml, left alone because that file is MANAGED by the scaffold and belongs in a template change.Gates, on the final tree
test-fast2379/2379,test-quality255/255, fulltask docsclean — 91 public and 22 internal bindings, no broken cross-references, no@examplefailures.The constituent PRs stay open for their detailed rationale; review this one.
This was opened by a bot. Please ping @seabbs for any questions.