refactor(did): typed run plan + recorded estimand/population metadata (#394 pillar 4) - #448
Open
ethanbuckley wants to merge 1 commit into
Open
refactor(did): typed run plan + recorded estimand/population metadata (#394 pillar 4)#448ethanbuckley wants to merge 1 commit into
ethanbuckley wants to merge 1 commit into
Conversation
…#394 pillar 4) Extends the ITT / gain-factor / level-factor run-plan pattern to the difference-in-differences family. A new did.py provides DiDModelSettings (fail-fast on a misspelled spec.extra key; validates the 9 legacy keys, incl. period_varying_dose requires dose) and a resolved DiDRunPlan that drives data preparation, factory construction and the config.json / model_recipe.md audit trail, recording the design, estimand, causal status, analysis population and missing-data assumption alongside every fit. fit_did now resolves and validates the plan before the context resets an output directory or the loader reads data, then consumes plan.prepare_kwargs() (which branches: binary -> t1-t3 levels frame; dose -> P1/P2 transition frame + attend) and plan.factory_kwargs() (period_varying_dose receives the resolved period_varying); reporting._resolved_run_plan gains a did branch so write_model_recipe and config.json's resolved_run_plan pick it up. Byte-identical: the resolved prepare + factory kwargs reproduce the former inline logic exactly for all 14 registered DiD models (verified programmatically against the old .get(...) semantics, incl. the outcomes default and the dose/binary branch). did-001 (binary) and did-006 (dose) dev fits are clean and now emit model_recipe.md plus a populated resolved_run_plan. 16 new run-plan tests; drift, factory, key-findings and prior-fallback suites pass; ruff / format:check / spellcheck clean. Part of #394 (pillar 4: typed settings + resolved plans). No estimand, likelihood, prior, population, fitted equation, sampling preset or artefact schema changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the difference-in-differences (DiD) statistical-model family to use a typed, validated settings + resolved run-plan boundary (mirroring the existing pattern in other families), so the plan is resolved before any output directory reset or data loading and is recorded into config.json / model_recipe.md for auditability.
Changes:
- Add
DiDModelSettings+DiDRunPlanwith strict legacy-key validation and pre-fit plan resolution. - Update
fit_didto driveload_and_prepare(...)andbuild_did_model(...)via the resolved plan (and persist the plan on the fit context). - Add a new test suite validating fail-fast behaviour and ensuring every registered DiD model resolves with required metadata.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/statistical_models/test_did_run_plan.py | Adds coverage for DiD typed settings/run-plan validation and “every registered model resolves with metadata” acceptance criteria. |
| src/language_reading_predictors/statistical_models/reporting.py | Teaches reporting to recover/emit a resolved DiD run plan for model_recipe.md and config.json. |
| src/language_reading_predictors/statistical_models/pipeline.py | Refactors fit_did to resolve/store the plan first and use plan.prepare_kwargs() / plan.factory_kwargs() downstream. |
| src/language_reading_predictors/statistical_models/did.py | Introduces the typed settings + resolved plan implementation and metadata/recipe generation for the DiD family. |
|
|
||
| def _resolved_run_plan(context: StatisticalFitContext): | ||
| """The typed run plan for families that have one (ITT, gain factors), else None.""" | ||
| """The typed run plan for families that have one (ITT, gain / did), else None.""" |
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
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.
Note
Drafted by an LLM-based AI tool (Claude Code/Opus 4.8).
#394 pillar 4 (typed settings + resolved plans), extending the pattern from the merged ITT / gain-factor (#418) and the level-factor (#445) run-plans to the difference-in-differences family. Independent, off
main.What this does
A new
did.pygives the DiD family the same typed boundary:DiDModelSettings— immutable, slotted, validated;from_legacy_extrarejects unknownspec.extrakeys (a misspelleddozefails before data loading), validates the 9 legacy keys, and enforcesperiod_varying_dose ⇒ dose.DiDRunPlan— resolved and validated before the context resets an output directory or the loader reads data. One object drivesload_and_prepare,build_did_model,model_recipe.md, andconfig.json, and records the design / estimand / causal status / analysis population / missing-data assumption. It branches correctly: binary → the t1–t3 levels frame (tau_t2the randomised DiD contrast); dose → the P1/P2 transition frame + theattendsession covariate (session coefficient observational).fit_didnow consumesplan.prepare_kwargs()/plan.factory_kwargs();reporting._resolved_run_plangains adidbranch so the recipe + config metadata are written.Byte-identical
Verified programmatically: for all 14 registered DiD models, the resolved
prepare_kwargsandfactory_kwargsreproduce the former inlinefit_didlogic exactly — including theoutcomesdefault ((outcome,)), the dose-vs-binary load branch, and the factory'speriod_varying_dose = dose AND flag. End-to-end:did-001(binary): levels frame, 159 obs, 0 divergences;did-006(dose): transition frame, 105 obs, 0 divergences;model_recipe.md+ a populatedresolved_run_plan(previously absent for DiD).16 new run-plan tests (
test_did_run_plan.py: fail-fast on unknown key +period_varying_dose⇒dose, typed-vs-legacy source, mixed-declaration rejection, binary/dose/off-floor/period-varying resolution, and every registered model resolving with metadata). Drift, factory, key-findings and prior-fallback suites pass;ruff/format:check/spellcheckclean.Scope
Mechanical, behaviour-preserving relocation only — no estimand, likelihood, prior, analysis population, fitted equation, sampling preset, diagnostic threshold or artefact schema change (per #394's contract). Next in the issue's suggested order after this: mediation, then adjusted/concurrent, then growth/historical/measurement.
Assigning to a human for review and merge.