fix(sdid): shape-only pre-treatment fit RMSE, level-gap field, placebo-anchored poor-fit warning - #818
Conversation
Overall assessmentExecutive summary
MethodologyP1 — Main-fit diagnostics bypass outcome normalizationLocation: Impact: Concrete fix: Calculate the residual using P3 — Warning threshold is an intentional library heuristicLocation: Impact: The Concrete fix: None required. The rule, absence of a paper/R anchor, and known limitations are explicitly documented. Code QualityNo additional findings. PerformanceNo findings; the added diagnostic work is linear in the number of pre-periods. MaintainabilityNo additional findings. Tests connect the fit, placebo, and sensitivity definitions at ordinary scales. Tech DebtP3 — Tutorial bootstrap comparison uses non-converged drawsLocation: Impact: The tutorial’s bootstrap robustness claim is not dependable when all 100 refits hit the iteration cap. Concrete fix: None required for approval; this is explicitly tracked in SecurityNo findings or apparent secrets. Documentation / TestsP2 — Tutorial overstates what good pre-fit establishesLocation: Impact: “Good fit means a trustworthy post-period effect” implies that pre-fit alone establishes identification, despite weighted parallel trends, no anticipation, and post-treatment shocks remaining unverified. Concrete fix: Say that good fit supports design plausibility but does not by itself validate the causal counterfactual. The required changelog fragment exists and passes the fragment checker. Path to Approval
|
igerber
changed the title
d3ba66f to
f485f66
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologyP3 — Placebo-draw coincidence claim needs qualificationLocation: Impact: The claim that diagnostic draws coincide with placebo-SE permutations is true for seeded unweighted/pweight fits, but not full-design surveys, whose SE loop uses stratified permutations. It is also not guaranteed when Concrete fix: Optionally qualify the statement to “for a fixed seed on non-full-design survey paths.” This is documentation-only; the unstratified diagnostic choice is explicitly documented and therefore not a defect. No estimator, variance, SE, or identification mismatch found. Code QualityNo findings. PerformanceNo findings. The additional work is intentionally capped at 20 Frank–Wolfe solves and documented. MaintainabilityNo findings. Tech DebtP3 — Geo tutorial uses non-converged bootstrap refitsLocation: Impact: The bootstrap robustness comparison is not dependable while all refits reach the iteration cap. Concrete fix: None required for approval; remediation is explicitly tracked in SecurityNo findings or apparent secrets. Documentation / TestsNo findings. The required |
f485f66 to
b512b54
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
MethodologyP3 — Placebo warning is a documented library heuristicLocation: Impact: The 20-draw pre-fit rank diagnostic is an SDiD adaptation of SCM placebo-fit practice, not a threshold prescribed by Arkhangelsky et al. or Abadie. The Registry explicitly documents the transposition, cap, survey behavior, and limitations, so this is not a defect. Concrete fix: None required. No estimator, weighting, variance, SE, or identification mismatch found. Code QualityNo findings. PerformanceNo findings. The additional diagnostic work is capped at 20 Frank–Wolfe solves and documented. MaintainabilityP1 — [Newly identified] Legacy pickles silently preserve the obsolete RMSE meaningLocation: Impact: Results pickled before this PR contain a level-inclusive Concrete fix: In Tech DebtP3 — Geo tutorial bootstrap refits do not convergeLocation: Impact: The displayed bootstrap CI is based on 100/100 non-converged refits. Concrete fix: None required for approval; remediation is tracked in SecurityNo findings or apparent secrets. Documentation / TestsP3 — Documentation overstates correspondence with the full solver objectiveLocation: Impact: Shape RMSE is the centered residual component; the full Frank–Wolfe objective also includes the ridge penalty. Concrete fix: Optionally replace “the quantity/objective the solver minimised” with “the data-fit component of the centered Frank–Wolfe objective.” Runtime tests were unavailable because NumPy, Pandas, and Pytest are not installed. AST parsing, notebook JSON parsing, diff checks, and changelog-fragment structure checks passed. Path to Approval
|
b512b54 to
5a9a973
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologyP3 — Documented placebo-warning heuristicLocation: Impact: The 20-draw rank diagnostic is a library adaptation of SCM placebo-fit practice, not a threshold prescribed for SDiD. The Registry explicitly documents this choice and its limitations, so it is not a defect. Concrete fix: None required. No estimator, weighting, variance, SE, or identification mismatch found. Code QualityNo findings. PerformanceNo findings. The additional diagnostic work is intentionally capped at 20 Frank–Wolfe solves. MaintainabilityThe previous P1 is resolved. Tech DebtP3 — Tracked tutorial convergence limitationLocation: Impact: The geo tutorial’s bootstrap refits do not converge, making that robustness cross-check unreliable. Concrete fix: None required for approval; remediation is tracked. SecurityNo findings or apparent secrets. Documentation / TestsP3 — Minor formula wording ambiguityLocation: Impact: “Recomputed as shape RMSE plus level gap” can imply the old level RMSE equals their arithmetic sum, which is false. Concrete fix: Say “the shape RMSE and level gap are recomputed from the stored trajectories.” P3 — Draw-count wording is internally inconsistentLocation: Impact: The Registry calls 20 the smallest draw count at which α=0.05 is reachable, while 19 draws yield Concrete fix: State that 19 is the minimum and 20 is the chosen cap/default. Runtime tests could not run because pytest, NumPy, and Pandas are unavailable. Python AST and notebook JSON parsing passed. |
…o-anchored poor-fit warning SyntheticDiD's pre_treatment_fit measured the raw level residual while the Frank-Wolfe unit weights are fit on column-centered outcomes, so a parallel treated series at a different level reported a large RMSE and a false poor-fit warning. The RMSE is now taken on the pre-period residual after removing its mean (the quantity the solver minimised), computed on the normalized arrays and rescaled; the removed mean is exposed as pre_treatment_level_gap. The poor-fit warning is anchored to an in-space placebo fit reference (Abadie, Diamond & Hainmueller 2010; Abadie 2021): up to 20 Algorithm-4 pseudo-treated draws refit at the fit-time zeta from a private RNG stream, warning when the placebo p-value <= 0.05; new fields pre_fit_placebo_rmse / pre_fit_placebo_pvalue. in_time_placebo and sensitivity_to_zeta_omega report the same shape-only statistic; DR/BR prose and the NaN (single pre-period) case are handled. Estimates, SEs and weights are unchanged. Tutorials 03/18 updated (18 re-executed); a pre-existing geo-tutorial Frank-Wolfe non-convergence surfaced by the refresh is tracked in TODO.md.
5a9a973 to
f423552
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologyP3 — Documented placebo-warning heuristicLocation: Impact: The capped placebo-rank warning is an explicitly documented SDiD adaptation of SCM placebo-fit practice, including its limitations and survey behavior. It is not a paper-prescribed SDiD threshold, but the documented deviation is informational only. Concrete fix: None required. No estimator, weighting, variance, SE, or identification mismatch found. Code QualityNo findings. PerformanceNo findings. The additional diagnostic computation is intentionally capped at 20 Frank–Wolfe solves. MaintainabilityNo findings. The prior P1 is resolved: legacy results recompute both shape RMSE and level gap from stored trajectories, or clear the obsolete metric when reconstruction is impossible ( Tech DebtP3 — Tracked tutorial convergence limitationLocation: Impact: The geo tutorial’s non-convergent bootstrap cross-check remains unreliable, but the pre-existing limitation is explicitly tracked. Concrete fix: None required for this PR. SecurityNo findings or apparent secrets. Documentation / TestsNo findings. The changelog wording now accurately describes trajectory-based reconstruction, and the Registry correctly states that 19 successful draws are the minimum for reaching α = 0.05. The required changelog fragment is present and valid. |
1 participant
Summary
SyntheticDiD.pre_treatment_fitand the "Pre-treatment fit is poor" warning measured the raw level residual between the treated mean and the synthetic control, while the Frank-Wolfe unit weights are fit on column-centered outcomes (intercept=True, matching Rsynthdid) and deliberately leave a constant level gap to the DiD step. A parallel treated series at a different level therefore reported a large RMSE and a false poor-fit warning even when the ATT was recovered exactly. The oldtest_poor_fit_emits_warningpinned exactly that false positive (treated ~100 vs controls ~10, both flat).pre_treatment_fitis now the shape-only RMSE (pre-period residual after removing its mean, i.e. the quantity the solver minimised), computed on the normalized arrays and rescaled byY_scale(cancellation-free; equals the multiplier-1.0sensitivity_to_zeta_omega()row exactly). The removed mean is exposed aspre_treatment_level_gap(signed, treated minus synthetic).in_time_placebo()andsensitivity_to_zeta_omega()report the same shape-onlypre_fit_rmse(column names unchanged). NaN with a single pre-period.1 x std(treated pre-outcomes)rule is unreachable for a shape-only RMSE (a flat synthetic against a trending treated series gives exactly the population SD). The warning now uses the in-space placebo fit assessment of Abadie, Diamond & Hainmueller (2010) / Abadie (2021): the treated fit is placed within the distribution of the same statistic over placebo fits in which a random set ofn_treatedcontrols is treated as if treated and omega is re-estimated on the rest at the fit-time zeta (steps 1-3 of Algorithm 4, warm-started like the placebo SE loop,min(n_bootstrap, 20)draws from a private RNG stream so SE draws are untouched). It fires whenpre_fit_placebo_pvalue = (1 + #{placebo >= treated}) / (1 + n_draws) <= 0.05, i.e. at the 20-draw default when the treated fit is worse than every placebo draw. New results fieldspre_fit_placebo_rmse/pre_fit_placebo_pvalue; the p-value is insummary()andto_dict(). Known limitation (inherent to any control-side reference, documented in the registry with the rejected alternatives): treated units far noisier than the controls fit worse than every placebo and warn; the warning text says so.summary()labels,to_dict(), DR (_pt_synthetic_fitskips on NaN instead of rendering "RMSE = nan"; narrative reworded) and the BR twin sentence, practitioner snippet,llms-full.txt, autosummary stub, REPORTING.md,docs/references.rst(Abadie 2021), tutorials 03 (source only, stays output-free) and 18 (re-executed; drift guards re-tightened; stakeholder markdown updated; "good fit" no longer claimed to validate the counterfactual).SyntheticControl(intercept=False, level RMSPE by design) is untouched.test_methodology_sdid.pygoes from ~218 s to ~330 s; a 50-draw cap doubled it, hence 20).TODO.md(Testing / docs): the geo tutorial's SDiD fits hit the Frank-Wolfe iteration cap under both backends onmainalready (the committed April outputs predate the aggregated non-convergence warning); surfaced by the notebook refresh, out of scope here.Methodology references (required if estimator / math changes)
synthdid(sc.weight.fw(intercept=TRUE),synthdid_rmse_plotplots the centered FW objective,synthdid_plotapplies an intercept offset)**Note:**indocs/methodology/REGISTRY.md(SyntheticDiD, unit-weights block) and in the "Poor pre-treatment fit" edge case.Validation
tests/test_methodology_sdid.py(TestPreTreatmentFitWarningrebuilt on the placebo p-value contract: trend-vs-flat misfit fires at p = 1/21; level-offset, flat-noise-level, single-pre-period NaN, single-control (no reference), noiseless-controls (placebo RMSE 0), treated-only level-shift invariance on all three surfaces, reference-field contract across variance methods incl. the n_bootstrap < 19 silence, and a 1e9 common-offset location-invariance test for the CI P1;test_pre_fit_rmse_recoverablerecovers both fields),tests/test_estimators.py(new fields +to_dictkeys),tests/test_diagnostic_report.py(single-pre-period fit -> PT analogueskipped, native diagnostics carry NaN).pytest --nbmake; 18 re-executed in place (shape RMSE 33.42, level gap 4.41, placebo p-value 0.381; the placebo SE moved 7.3125 -> 7.2659 because the committed outputs came from an older environment - HEAD's code gives the same 7.2659 here).Security / privacy
Changelog
changelog.d/20260904-sdid-shape-only-fit-rmse.md: Fixed / Behavioral Changes / Added)