Add SyntheticDiD validation diagnostics

[igerber]

Summary

• New post-fit diagnostics on SyntheticDiDResults: get_loo_effects_df(), get_weight_concentration(), in_time_placebo(), sensitivity_to_zeta_omega(), plus synthetic/treated pre and post trajectories retained as public fields.
• Private _SyntheticDiDFitSnapshot on results (Y matrices, unit IDs, period lists, survey weights) so re-estimation diagnostics work from the results object without requiring the user to re-supply panel data. Arrays are marked read-only after fit.
• Jackknife LOO entries in placebo_effects can now be mapped back to user-facing unit identities; get_loo_effects_df() returns the joined view sorted by influence.
• _handle_synthetic in practitioner.py rewritten to reference the new callables instead of pseudo-code comments.
• REGISTRY.md gains a Validation diagnostics subsection documenting purpose, formulas, and edge cases for each diagnostic; doc-deps.yaml links results.py to the methodology entry.

Methodology references (required if estimator / math changes)

• Method name(s): SyntheticDiD (post-fit diagnostics; core estimator and variance math unchanged).
• Paper / source link(s): Arkhangelsky, D., Athey, S., Hirshberg, D. A., Imbens, G. W., & Wager, S. (2021). Synthetic Difference-in-Differences. American Economic Review, 111(12), 4088-4118.
• Any intentional deviations from the source (and why):
  • in_time_placebo() reuses self.zeta_omega / self.zeta_lambda by default instead of recomputing auto-zeta per fake window (matches R synthdid convention of treating regularization as a property of the original fit). Labelled **Note:** in REGISTRY.md.
  • sensitivity_to_zeta_omega() holds the original Frank-Wolfe time weights fixed to isolate sensitivity to zeta_omega; zeta_lambda sensitivity is not exposed. Labelled **Note:** in REGISTRY.md.

Validation

• Tests added/updated: tests/test_methodology_sdid.py gains 33 new tests across TestFitSnapshot, TestTrajectories, TestLooEffectsDf, TestWeightConcentration, TestInTimePlacebo, TestSensitivityToZetaOmega, TestPractitionerSdidReferences - covering snapshot shapes and read-only contract, unweighted and survey-weighted trajectories, LOO DataFrame correctness (positional mapping, NaN propagation, ValueError on non-jackknife variance), concentration metrics under composed survey weights, negative top_k validation, in-time placebo default sweep feasibility, empty-default schema preservation, explicit-list override, zeta refit regression at multiplier=1.0, and the exact documented default grid values.
• Backtest / simulation / notebook evidence (if applicable): N/A - diagnostics only; no estimator math changes.

Security / privacy

• Confirm no secrets/PII in this PR: Yes

[github-actions]

Overall Assessment

⚠️ Needs changes

Executive Summary

• Affected methods: SyntheticDiDResults.get_loo_effects_df(), get_weight_concentration(), in_time_placebo(), and sensitivity_to_zeta_omega(). I did not find an unmitigated estimator, weighting, or SE-methodology regression in the changed SyntheticDiD fit path.
• P1: sensitivity_to_zeta_omega() does not handle empty grids correctly. zeta_grid=[] or multipliers=() returns a zero-column DataFrame, not the documented schema.
• The two math-adjacent diagnostic deviations are documented in REGISTRY.md, so they are P3 informational only.
• New tests cover many happy paths and several edge cases, but they miss the empty-grid branch that would have caught the P1 above.
• This was a static review only; I could not execute pytest in this environment because numpy/pandas are unavailable.

Methodology

• Severity: P3-informational. Impact: the only computation-level deviations I found are the documented ones: in_time_placebo() reuses the fitted zetas, and sensitivity_to_zeta_omega() holds the original time weights fixed. Those are explicitly noted in docs/methodology/REGISTRY.md:1521 and docs/methodology/REGISTRY.md:1524, while the core ATT/SE path remains the same code path in diff_diff/synthetic_did.py:474 and diff_diff/synthetic_did.py:513. Concrete fix: none; no action required. citeturn0search1turn0search2turn1search2

Code Quality

• Severity: P1. Impact: sensitivity_to_zeta_omega() promises a result with columns zeta_omega, att, pre_fit_rmse, max_unit_weight, and effective_n, but when the resolved grid is empty it returns pd.DataFrame(rows) with rows=[], i.e. a zero-column frame. Any downstream caller that branches on an empty grid but still expects the documented schema will break. Location: diff_diff/results.py:1290 through the return at diff_diff/results.py:1361; documented contract at docs/methodology/REGISTRY.md:1524. Concrete fix: predeclare columns = ["zeta_omega", "att", "pre_fit_rmse", "max_unit_weight", "effective_n"] and return pd.DataFrame(columns=columns) when zeta_values is empty; construct non-empty results with the same column list.

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• Severity: P3-informational. Impact: the committed docs surfaces still advertise the old SyntheticDiDResults API, e.g. docs/llms-full.txt:1030 and docs/api/_autosummary/diff_diff.SyntheticDiDResults.rst:16. This is already consistent with the tracked docs debt in TODO.md:95 and TODO.md:99, so I’m not treating it as a blocker. Concrete fix: none required for approval; refresh generated docs as part of the existing TODO workstream.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: the new sensitivity tests cover only non-empty grids, so the empty-result regression above is untested. See tests/test_methodology_sdid.py:1862 through tests/test_methodology_sdid.py:1928. Concrete fix: add regressions for res.sensitivity_to_zeta_omega(zeta_grid=[]) and res.sensitivity_to_zeta_omega(multipliers=()), asserting zero rows with the exact documented column set.
• Validation note: I could not run the test suite locally because this review environment is missing numpy and pandas.

Path to Approval

1. In sensitivity_to_zeta_omega(), preserve the documented output schema when the resolved zeta grid is empty.
2. Add regression tests for both zeta_grid=[] and multipliers=() that assert shape == (0, 5) and the exact expected column names.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 0843481739c6b6bd31511a7a18f5fece5ac6e2e3

---

Overall Assessment

✅ Looks good

Executive Summary

• Re-review: the prior P1 on empty sensitivity_to_zeta_omega() outputs is fixed in diff_diff/results.py:1314 and covered by tests/test_methodology_sdid.py:1930 and tests/test_methodology_sdid.py:1942.
• Affected methodology is limited to post-fit SyntheticDiD diagnostics and retained fit state; I did not find a new ATT, weighting, or variance regression in the estimator path. The only behavior deviations I found are the two explicitly documented registry notes for in_time_placebo() and sensitivity_to_zeta_omega(). (synth-inference.github.io)
• P2: the new practitioner jackknife snippet can still raise on valid variance_method="jackknife" results when LOO estimates are unavailable, because it checks only results.variance_method before calling get_loo_effects_df().
• P2: _fit_snapshot now retains outcome matrices, unit IDs, and survey weights on every SyntheticDiDResults, which increases accidental data-retention / serialization risk.
• Validation note: this was a static review only; pytest is unavailable here, and the environment also lacks numpy and pandas.

Methodology

• Severity: P3 informational. Impact: the only methodology-level differences I found are the documented ones in docs/methodology/REGISTRY.md:1521 and docs/methodology/REGISTRY.md:1524: in_time_placebo() reuses fitted zetas by default, and sensitivity_to_zeta_omega() holds lambda fixed. The changed fit path still computes ATT and inference through the existing estimator / variance code at diff_diff/synthetic_did.py:474, diff_diff/synthetic_did.py:513, and diff_diff/synthetic_did.py:559. Concrete fix: none; no action required. (synth-inference.github.io)

Code Quality

• Severity: P2. Impact: the shipped practitioner snippet at diff_diff/practitioner.py:548 only gates on results.variance_method == 'jackknife' before calling get_loo_effects_df(), but SyntheticDiD legitimately returns empty jackknife output for single-treated or single-effective-control designs at diff_diff/synthetic_did.py:1247 and diff_diff/synthetic_did.py:1267, and get_loo_effects_df() raises in that state at diff_diff/results.py:1018. Concrete fix: guard on LOO availability (results.placebo_effects is not None or getattr(results, "_loo_unit_ids", None) is not None) or catch ValueError and print a graceful fallback message instead. This is a real edge case, not a theoretical one; the official synthdid variance docs also call out single-treated designs as special handling. (synth-inference.github.io)

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• No new findings.

Security

• Severity: P2. Impact: the new _SyntheticDiDFitSnapshot retains outcome matrices, unit IDs, and survey weights on the results object at diff_diff/results.py:653, and every fit now attaches it unconditionally at diff_diff/synthetic_did.py:593 and diff_diff/synthetic_did.py:618. That makes SyntheticDiDResults materially more sensitive than before; generic pickling or dataclasses.asdict() will now carry underlying panel state even though the new method errors describe serialization as excluding the snapshot at diff_diff/results.py:1099 and diff_diff/results.py:1276. Concrete fix: add an explicit serialization guard that drops _fit_snapshot, or make snapshot retention opt-in and document the privacy trade-off in the public API.

Documentation/Tests

• Severity: P2. Impact: the new practitioner tests only assert that the snippets reference existing methods and parse as Python at tests/test_methodology_sdid.py:1958 and tests/test_methodology_sdid.py:1985; they do not exercise the jackknife-unavailable branch that would have caught the practitioner failure above. Concrete fix: add a regression on a one-treated-unit SyntheticDiD(variance_method="jackknife") result and assert the generated guidance produces a graceful fallback message instead of invoking get_loo_effects_df().
• Validation note: I could not run the tests here because pytest is missing and the environment also lacks numpy and pandas.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: a5c080c6370acb17cee734934093d0f3e12f83f2

---

Overall Assessment

✅ Looks good

Executive Summary

• Re-review focus: the prior practitioner failure is fixed. The jackknife guidance now checks LOO availability before calling get_loo_effects_df(), and there is a regression covering the single-treated-unit case in diff_diff/practitioner.py:L539-L557 and tests/test_methodology_sdid.py:L1998-L2024.
• The methodology impact is limited to post-fit SyntheticDiDResults diagnostics (get_loo_effects_df, trajectories, get_weight_concentration, in_time_placebo, sensitivity_to_zeta_omega). I did not find a regression in the ATT or SE paths, which still run through the existing SDID estimator and variance code in diff_diff/synthetic_did.py:L474-L647.
• The only behavior deviations I found are the two explicitly documented registry notes: in_time_placebo() reuses the original fit’s zetas, and sensitivity_to_zeta_omega() holds time weights fixed. Those are documented in docs/methodology/REGISTRY.md:L1521-L1526, so they are informational rather than defects.
• Residual P2: the new pickle guard is real, but _fit_snapshot is still a declared dataclass field. That means pickle is protected while other dataclass-recursive serialization paths can still carry the retained panel matrices, unit IDs, and survey weights from diff_diff/results.py:L780-L810 and diff_diff/synthetic_did.py:L593-L647.
• P3 informational: the checked-in autosummary stub for SyntheticDiDResults still omits the new public diagnostics/trajectory fields in docs/api/_autosummary/diff_diff.SyntheticDiDResults.rst:L14-L53. That is already tracked as stale autosummary tech debt in TODO.md:L88-L99.
• Validation note: this was a static review only. The environment here is missing numpy, pandas, and pytest, so I could not execute the test suite.

Methodology

• Severity: P3 informational. Impact: the affected methods are the new post-fit SDID diagnostics and retained trajectory/snapshot state, not the core estimator or variance formulas. The fit path still computes ATT and inference through the existing SDID estimator, placebo/bootstrap/jackknife branches, and safe_inference() in diff_diff/synthetic_did.py:L474-L573, and the in-repo SDID references still point to Arkhangelsky et al. (2021) / Algorithms 3 and 4 in diff_diff/synthetic_did.py:L26-L58 and diff_diff/synthetic_did.py:L1189-L1382. The two deviations I found are both documented in the Methodology Registry: zeta reuse in in_time_placebo() and fixed time weights in sensitivity_to_zeta_omega() at docs/methodology/REGISTRY.md:L1521-L1526. Concrete fix: none.

Code Quality

• Severity: P3 informational. Impact: the previous re-review code-quality concern around practitioner guidance is addressed. _handle_synthetic() no longer assumes variance_method='jackknife' implies LOO output is available, and the new regression executes the snippet against a valid single-treated-unit jackknife result without raising in diff_diff/practitioner.py:L539-L557 and tests/test_methodology_sdid.py:L1998-L2024. Concrete fix: none.

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• No new untracked tech-debt findings.

Security

• Severity: P2. Impact: _fit_snapshot is now excluded from pickle via __getstate__(), but it remains a declared dataclass field on SyntheticDiDResults in diff_diff/results.py:L780-L810, and every fit still attaches the full retained panel snapshot in diff_diff/synthetic_did.py:L593-L647. That means pickle round-trips are safer, but generic dataclass-recursive serializers still have a straightforward path to copy the stored outcome matrices, unit IDs, and survey weights. The new tests only cover pickle behavior in tests/test_methodology_sdid.py:L2027-L2078. Concrete fix: remove _fit_snapshot from the dataclass field list and attach it as a plain private attribute after constructing SyntheticDiDResults, or provide a dedicated safe-export path and explicitly document dataclasses.asdict()-style serialization as unsupported for these results.

Documentation/Tests

• Severity: P3 informational (tracked in TODO.md). Impact: the tracked autosummary page for SyntheticDiDResults still lists only the pre-existing methods/attributes in docs/api/_autosummary/diff_diff.SyntheticDiDResults.rst:L14-L53, so the checked-in API docs lag the new public diagnostics unless the stubs are refreshed. This matches the already-tracked stale-autosummary item in TODO.md:L88-L99, so it is not a blocker. Concrete fix: regenerate docs/api/_autosummary/* or stop versioning those stubs and treat them as build output.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: f0c9e5c8e63412418a07c0b041cb37a9cf5c0bad

---

Overall Assessment

✅ Looks good

Executive Summary

• Re-review focus: the prior practitioner failure is fixed. _handle_synthetic() now guards the jackknife LOO snippet on _loo_unit_ids instead of unconditionally calling get_loo_effects_df() in diff_diff/practitioner.py#L548, with regression coverage in tests/test_methodology_sdid.py#L1998.
• The methodology impact is limited to post-fit SDID diagnostics on SyntheticDiDResults; the ATT and SE paths still run through the existing estimator, variance methods, and safe_inference() in diff_diff/synthetic_did.py#L474 and diff_diff/synthetic_did.py#L1193. That is consistent with the SDID paper and the official synthdid variance/diagnostic workflow. (aeaweb.org)
• The prior serializer/privacy concern is addressed: _fit_snapshot and _loo_* are no longer dataclass fields, __getstate__() strips the fit snapshot on pickle, and regressions cover pickle, dataclasses.fields(), and dataclasses.asdict() in diff_diff/results.py#L786, diff_diff/results.py#L802, and tests/test_methodology_sdid.py#L2027.
• The only remaining item I found is informational: the two methodology deviations are explicitly documented in docs/methodology/REGISTRY.md#L1521, and the stale autosummary stub for SyntheticDiDResults remains tracked in TODO.md#L88.
• Validation note: this was a static review only. I could not run pytest here because the environment is missing pytest and numpy.

Methodology

• Severity: P3 informational. Impact: the affected methods are post-fit diagnostics only (get_loo_effects_df(), trajectories, get_weight_concentration(), in_time_placebo(), sensitivity_to_zeta_omega()), not estimator or variance changes; the underlying SDID estimator and variance surface remain aligned with Arkhangelsky et al. and official synthdid, and official synthdid already exposes weight, trajectory, and placebo-inspection surfaces. The only deviations I found are the REGISTRY-noted zeta reuse in docs/methodology/REGISTRY.md#L1521 and fixed time weights in docs/methodology/REGISTRY.md#L1524, so this is non-blocking. Concrete fix: none. (aeaweb.org)

Code Quality

• Severity: none. Impact: no code-quality defects identified in the changed paths; the earlier practitioner regression is fixed by the guarded LOO snippet in diff_diff/practitioner.py#L550 with regression coverage in tests/test_methodology_sdid.py#L1998. Concrete fix: none.

Performance

• Severity: none. Impact: no performance regression stood out in the changed code. The new re-estimation helpers are opt-in result methods, and the main fit() path only adds snapshot/trajectory retention in diff_diff/synthetic_did.py#L593. Concrete fix: none.

Maintainability

• Severity: none. Impact: no maintainability defect identified; the diagnostic state is localized via _SyntheticDiDFitSnapshot and attached after results construction in diff_diff/results.py#L652 and diff_diff/synthetic_did.py#L622. Concrete fix: none.

Tech Debt

• Severity: none. Impact: no new untracked tech-debt finding. Concrete fix: none.

Security

• Severity: none. Impact: no new security finding; the prior serialization concern is addressed by the plain-attribute _fit_snapshot/_loo_* setup and pickle guard in diff_diff/results.py#L786 and diff_diff/results.py#L802, with regression coverage in tests/test_methodology_sdid.py#L2037 and tests/test_methodology_sdid.py#L2080. Concrete fix: none.

Documentation/Tests

• Severity: P3 informational (tracked in TODO.md). Impact: the checked-in autosummary stub for SyntheticDiDResults still omits the new diagnostics and trajectory members in docs/api/_autosummary/diff_diff.SyntheticDiDResults.rst#L14 and docs/api/_autosummary/diff_diff.SyntheticDiDResults.rst#L30. This remains explicitly tracked in TODO.md#L99, so it is not a blocker. Concrete fix: regenerate docs/api/_autosummary/* or stop versioning those stubs.