feat!: remove default_prior, param_priors and build_priors — priors are not this package's job - #373
feat!: remove default_prior, param_priors and build_priors — priors are not this package's job#373seabbs-bot wants to merge 5 commits into
Conversation
Priors are DistributionsInference.jl's job, not ComposedDistributions'. Bare uncertain(tree) now marks every free parameter no_prior() instead of guessing a support-derived default, including a Resolve node's branch-probability simplex. Review follow-up on this branch: - Reworded the no_prior()/rand guarantee to scope it to an Uncertain leaf; a Resolve node's branch_prob_prior does not yet carry the same refuse-on-draw guard (filed as #366, referenced from NEWS.md and no_prior.jl). - Dropped a duplicate ConvolvedDistributions [sources]/compat change that belongs to the separate, already in-flight #359. - Fixed a docstring detachment bug Aqua's undocumented-names check caught: _is_spec_value had been inserted between Uncertain's @doc block and the struct it documents, silently stealing the docstring. Moved it above the docstring instead. - Ran task format and confirmed task test-quality passes clean (233/233).
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/free-uncertain-drop-prior-machinery")
using ComposedDistributionsOption 2: Local CheckoutIf you have the repo locally: git checkout feat/free-uncertain-drop-prior-machinery
julia --project=. -e "using Pkg; Pkg.instantiate()" |
Both Uncertain's eager unresolved-spec check in Base.rand and Base.rand(::AbstractRNG, ::NoPrior) are new on this branch and both worked undetected by all existing tests: neither guard's deletion would have failed a single test. Verified by deleting each guard in turn, watching these new tests fail, then restoring it.
…_prior A Resolve whose branch_prob_prior is marked no_prior() still draws silently from its fixed branch_probs on every rand path (standalone, outcome-pair, and nested in a tree) — exactly the class of silence the leaf-level Uncertain guard exists to remove. Red: fails against current Resolve.jl (no guard yet).
A Resolve whose branch_prob_prior was marked no_prior() drew silently from its fixed branch_probs, ignoring the marked uncertainty -- the same class of silence Uncertain's leaf-level rand guard already refuses. _check_branch_probs_resolved(c::Resolve) mirrors that guard's error style and message shape exactly, naming the node and pointing at uncertain(tree; branch_probs = prior, ...) / update(tree, table) / update(tree, params). It runs in both rand entry points that read branch_probs directly: _rand_outcome (the standalone named-record and outcome-pair draws) and _one_of_marginal_rand (the in-tree flat-value path via child_rand!). A fixed node (branch_prob_prior === nothing) or one carrying a resolved Dirichlet is unaffected: the pre-existing gap where rand does not yet draw fresh branch probabilities from an attached Dirichlet (noted in the issue) stays out of scope for this fix.
|
Two gaps from the batch report are now closed on this branch. #366 is fixed (961f26c). A The missing guard test is added (6d64fec). Both followed red/green TDD — the #366 reproduction (275e342) is a real red commit that fails against the unguarded Still draft, for one reason only. The open question is whether "estimated" is carried by the Gates: This comment was posted by a bot. Please ping @seabbs for any questions. |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
📖 Documentation preview is ready! View the docs for this PR at: https://EpiAware.github.io/ComposedDistributions.jl/previews/PR373/ This preview will be updated automatically when you push new commits. |
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)
|
…n/0.2.0-contract (#381) Folds the prior-removal work into the leaf/node contract branch so the breaking 0.2.0 surface lands as one PR. Kept #373's NoPrior-in-prior-column design (confirmed) layered on #381's composed_to_table/full-inventory rewrite; removed build_priors/default_prior/param_priors and their dedicated test coverage throughout.
|
Folded into #381 (2026-08-14), which now carries this branch's prior-removal work merged on top of the leaf/node contract rewrite. The open Closing in favour of #381; this branch stays for the detailed rationale. This was closed by a bot. Please ping @seabbs for any questions. |
These sat outside every conflict marker (git's 3-way merge never flagged them), so the earlier merge-conflict sweep didn't catch them. Caught by running task test-fast against the actual committed tree instead of the locally-fixed-but-uncommitted working copy CI would not see.
2 participants
Priors leave this package for DistributionsInference. Structure, supports and which parameters are free stay here.
What changes
default_prior,param_priorsandbuild_priorsare removed with no replacement. A parameter's name does not determine its family — anInverseGaussian'smuis positive, aSkewNormal's shape is signed — so support-and-name-keyed prior guessing was wrong in exactly the cases where it mattered.Bare
uncertain(tree)still means estimate-everything. It now marks each parameterno_prior()rather than inventing a distribution for it. To attach priors: takecomposed_to_table(tree), fill thepriorcolumn,update(tree, tbl).randon anUncertainleaf still carrying an unresolvedno_prior()throws, naming the parameter, instead of silently drawing from a template.logpdfon such a tree throws where it previously returned the template's value. The fitting path is unaffected —reconstructcollapses every spec first.Resolve.branch_prob_prioracceptsno_prior(), and bareuncertain(tree)no longer injectsDirichlet(ones(K))on your behalf. A user-suppliedDirichletbehaves exactly as before.Gates
test-fast1692/1692,test-quality233/233, clean fulldocs/make.jl, formatter clean. All three documentedrand-refusal behaviours reproduced by direct execution.Project.tomlis unchanged from main.Draft, for two reasons
One test is missing and it is the important one.
Uncertain's eagerNoPriorcheck andBase.rand(::AbstractRNG, ::NoPrior)are both new here, both work today, and both would survive their own deletion undetected by all 1925 tests. The guard is the safety property this PR exists to add, so it needs a red test pinning it before merge.One design question is yours (Q4 in the batch report). The marker lives **in the
priorcolumn**, soprior !== nothingstill means estimated and all six downstream consumers keep working unchanged — DI'sestimated_rowsandrequired_parameters, theflat_dimension == count(!isnothing, prior)invariant,CentredPoolPrior, table round-trip. The alternative is a newestimated::Boolcolumn, which is cleaner on principle — storing a non-prior in a column named prior is theCentredPoolPriormistake again — but means rebasing a new column through #343's rewritten walk and its 16 golden fixtures.If you keep the column marker, two conditions: re-document the column as holding a prior or a marker, and make DI raise an explicit error when a
NoPriorreachesflat_priors, since today it wouldMethodErroronlogpdf.Known hole
#366: a
Resolvewhosebranch_prob_priorisno_prior()still draws silently from its fixedbranch_probs— exactly the class of silence this PR exists to remove. Recommend fixing it here rather than shipping it documented.Refs #332.
This was opened by a bot. Please ping @seabbs for any questions.