Skip to content

feat: publish the reserved per-record field registry + misspelling guard (#262)#299

Merged
seabbs-bot merged 1 commit into
mainfrom
feat/262-reserved-key-registry
Jul 21, 2026
Merged

feat: publish the reserved per-record field registry + misspelling guard (#262)#299
seabbs-bot merged 1 commit into
mainfrom
feat/262-reserved-key-registry

Conversation

@seabbs-bot

Copy link
Copy Markdown
Collaborator

Part of #262 (the additive, non-conflicting half).

What

  • Public reserved_record_fields() — publishes the reserved per-record field
    names (weight/count/obs_time/obs_window/branch_probs/branch_prob)
    that a scoring row carries for their own meaning rather than as events,
    documented with each field's owner, type, default and when it is read, and a
    note that the covariate context is a separate open namespace.
  • A row that misspells a reserved field (e.g. obs_tim) now raises a clear
    error naming the reserved field (bounded edit-distance near-miss), instead of
    the bare "row field :obs_tim is not an event of this tree".

Additive: valid records — including those carrying reserved fields — score
exactly as before. Only tree_events.jl is touched (plus the export + a test);
no change to the covariate/Context code.

Scope note: the merge-overwrite guard is deferred (design decision on #262)

#262 also asks to "raise an explicit error when the context merge would overwrite
a key." That half is not in this PR: a blanket overwrite-error conflicts with
released, tested behaviour — test/composers/varying.jl:243 asserts
with_covariates(Context(a=1); a=9).covariates.a == 9 ("later keys win"), and the
sampler uses the same merge to thread latent params — and at the merge level an
accidental collision is indistinguishable from an intentional update. I posted
the semantics question (break / opt-in strict merge / defer) on #262 for review.
Deliberately keeping this PR off varying.jl also avoids the in-flight
#257/#266 covariate lane.

Tests

test/composers/reserved_fields.jl (4 @testitems): the registry is published;
a valid record (with a reserved field) scores unchanged; a misspelled reserved
field errors helpfully while a genuine unknown field does not get a false hint;
and the near-miss/edit-distance unit checks. task test-fast and
task test-quality pass.

This was opened by a bot. Please ping @seabbs for any questions.

@seabbs-bot

Copy link
Copy Markdown
Collaborator Author

Review: independent pass (cd-reviewer-2)

Ran from a throwaway worktree of 9837dce (feat/262-reserved-key-registry).

No-behaviour-change claim — confirmed

The diff only restructures the error branch of _row_event_vector_by_name
(k in enames || throw(...) becomes k in enames && continue followed by a
richer throw): the success path (continue for a reserved or valid-event
field) is untouched. Confirmed empirically too — logpdf(c, rec) and
logpdf(c, merge(rec, (weight = 2.0,))) give bit-identical scores to the
pre-existing baseline.

Edit-distance false-positive surface — the important one, checked directly

Built a tree with a genuine event actually named :weights (edit-distance 1
from the reserved :weight) and scored a record carrying it:

tree = sequential(:onset => Gamma(2.0, 1.0), :weights => Gamma(1.5, 1.0))
logpdf(tree, rand(Xoshiro(1), tree))  # scores fine, no error, no hint

This can't go wrong structurally: k in enames && continue is checked
before _reserved_near_miss, so a field that's a genuine event of this
tree never reaches the near-miss guard at all, regardless of how close its
name is to a reserved word. The only way to see a "did you mean" hint is for
a field to already be invalid (neither reserved nor a declared event) —
i.e. a case that already threw before this PR. The one soft edge is
cosmetic: a genuinely-intended-but-unsupported field that happens to be near
a reserved word (e.g. :countx) gets a suggestion that doesn't apply — but
the error itself is unchanged (already thrown pre-PR), so this is a message-
quality nuance, not a behavioural regression.

New public accessor docstring

reserved_record_fields() takes no arguments, so it correctly has no
# Arguments section (the org's EpiAwarePackageTools QA linter only
requires that section when the function actually has positional args — I
confirmed this reading its qa.jl source during a prior review). It does
have a proper ```@example ``` block. Rendered the docstring directly via
@doc and confirmed it displays correctly (including the δ in the
obs_window description) — the raw"..." string literal doesn't mangle
anything.

NEWS wording on the deferred merge-guard

"the covariate-context merge-overwrite guard the issue also raises is a
separate design decision left open" — appropriately non-committal, matches
the PR body's own framing ("I posted the semantics question ... on #262 for
review"). Doesn't promise or presume an outcome Sam hasn't ruled on.

Suites + hygiene

Verdict

Approve. Narrow, additive, well-tested change. The false-positive concern
I was asked to scrutinise doesn't materialise — valid events are resolved
before the near-miss guard can ever fire — and the NEWS/PR framing correctly
avoids pre-empting Sam's still-open call on the merge-overwrite guard.

…ard (#262)

Add a public `reserved_record_fields()` accessor returning the reserved
per-record field names (weight/count/obs_time/obs_window/branch_probs/
branch_prob), documented with each field's owner, type, default and when it is
read, plus a note that the covariate context is a separate open namespace.

A row that misspells a reserved field (e.g. `obs_tim`) now raises a clear error
naming the reserved field, via a bounded edit-distance near-miss check, instead
of the bare "not an event of this tree". Additive: valid records (including
reserved fields) score exactly as before.

This is the non-conflicting half of #262. The covariate-context merge-overwrite
guard the issue also raises conflicts with released, tested last-writer-wins
behaviour (test/composers/varying.jl) and is left open as a design decision
(posted on #262 for review); this PR touches only tree_events.jl, not the
covariate/Context code, to avoid the in-flight #257/#266 lane.

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
@seabbs-bot
seabbs-bot force-pushed the feat/262-reserved-key-registry branch from 9837dce to 795f7d4 Compare July 21, 2026 11:17
@seabbs-bot
seabbs-bot merged commit 78034f7 into main Jul 21, 2026
8 of 19 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Try this Pull Request!

Option 1: Julia Package Manager

Open Julia and type:

import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/EpiAware/ComposedDistributions.jl", rev="feat/262-reserved-key-registry")
using ComposedDistributions

Option 2: Local Checkout

If you have the repo locally:

git checkout feat/262-reserved-key-registry
julia --project=. -e "using Pkg; Pkg.instantiate()"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant