Skip to content

feat(synthetic_control): split cv-infeasible from failed in placebo/LOO diagnostics#598

Merged
igerber merged 1 commit into
mainfrom
feature/sc-placebo-infeasible-reason-code
Jul 2, 2026
Merged

feat(synthetic_control): split cv-infeasible from failed in placebo/LOO diagnostics#598
igerber merged 1 commit into
mainfrom
feature/sc-placebo-infeasible-reason-code

Conversation

@igerber

@igerber igerber commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • Under v_method="cv", an excluded in_space_placebo() / leave_one_out() refit whose (pseudo-treated / reduced) donor pool is indistinguishable in a re-aggregated CV window is now tallied as a structural "infeasible" exclusion — distinct from a solver "failed" non-convergence — mirroring the split in_time_placebo already reports.
  • _outer_solve_V_cv returns a structural-infeasible flag; _placebo_fit_unit returns a (result, status) tuple (status ∈ {ran, infeasible, failed}).
  • in_space_placebo gains a public n_infeasible field; leave_one_out gains _loo_n_infeasible. Statuses gain all_placebos_infeasible / all_placebos_unusable (resp. all_refits_*); the per-row status column carries "infeasible"; DiagnosticReport surfaces a machine-readable reason_code + n_failed / n_infeasible for both surfaces.
  • The permutation placebo_p_value / n_placebos are unchanged (both causes are excluded from the rank / ATT range identically) — only the diagnostic attribution is refined. n_infeasible is 0 for the non-cv v_methods.
  • SyntheticControlResults.__setstate__ backfills the new counters to 0 on legacy unpickle, so summary() / to_dict() never AttributeError on a result pickled by an older version.

Methodology references (required if estimator / math changes)

  • Method name(s): SyntheticControl in-space placebo permutation inference + ADH-2015 §4 leave-one-out — diagnostic reporting only (the ADH 2010 §2.4 rank test, donor weights, and estimand are unchanged).
  • Paper / source link(s): Abadie, Diamond & Hainmueller (2010, 2015); the CV window-identification gate follows Abadie (2021) fn. 7.
  • Any intentional deviations from the source (and why): None. Diagnostic-accounting refinement, documented in docs/methodology/REGISTRY.md (§SyntheticControl, "in-space / leave-one-out infeasible vs failed split").

Validation

  • Tests added/updated: tests/test_methodology_synthetic_control.py — real v_method="cv" structural-infeasible discriminators for in-space (test_cv_in_space_placebo_excludes_donor_flat_refits, refined) and leave-one-out (test_cv_leave_one_out_flat_reduced_pool_infeasible, new real-DGP), the new all_*_infeasible / all_*_unusable status codes, a legacy-unpickle backfill test, and an extended pickle round-trip. Existing _placebo_fit_unit monkeypatch stubs updated to the new (result, status) contract.
  • Backtest / simulation / notebook evidence (if applicable): N/A (no tutorial changes).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected methods: SyntheticControl in-space placebo inference, ADH-2015 leave-one-out diagnostics, and the shared _placebo_fit_unit() refit path.
  • The infeasible-vs-failed split is documented in docs/methodology/REGISTRY.md:L2223, so it is not an undocumented methodology deviation.
  • The PR preserves the rank/denominator contract: failed and infeasible refits are both excluded from placebo_p_value / LOO ATT range.
  • Tuple-return propagation is updated at the visible direct callers.
  • Minor user-facing documentation/reason text gaps remain.
  • Verification not run: pytest is not installed in this environment.

Methodology

Finding: P3-informational — documented diagnostic-accounting refinement
Impact: The PR changes attribution of excluded CV refits, not the estimator, donor weights, estimand, permutation rank, or ATT range. This is documented as a Registry Note at docs/methodology/REGISTRY.md:L2223, and the implementation excludes both "failed" and "infeasible" refits identically from the rank/range in diff_diff/synthetic_control_results.py:L951-L1063 and diff_diff/synthetic_control_results.py:L1203-L1294.
Concrete fix: None required.

Code Quality

No findings.

Performance

No findings. The change threads status from existing refits; it does not introduce additional solver runs.

Maintainability

Finding: P3 — confidence-set error reason map does not know the new no-reference statuses
Impact: _require_placebo_reference() still only names all_placebos_failed; if all placebos are structurally infeasible or mixed unusable, test_sharp_null() / confidence_set() raise a generic “no valid reference set” message. Computation still fails closed correctly, so this is not a methodology or correctness blocker. Location: diff_diff/synthetic_control_results.py:L1700-L1739.
Concrete fix: Add all_placebos_infeasible and all_placebos_unusable to the reasons dict and update the docstring line describing no-reference causes.

Tech Debt

No findings.

Security

No findings.

Documentation/Tests

Finding: P3 — LOO return docstring omits status="infeasible"
Impact: leave_one_out() can now return excluded rows with status="infeasible", but the Returns text still says dropped rows are "loo" or "failed" and that failed rows sort last. Location: diff_diff/synthetic_control_results.py:L1109-L1115.
Concrete fix: Update the docstring to list "infeasible" as a NaN-metric excluded status and say excluded rows sort last.

…OO diagnostics

Under v_method="cv", an excluded in_space_placebo() / leave_one_out() refit whose
(pseudo-treated / reduced) donor pool is indistinguishable in a re-aggregated CV
window is now tallied as a structural "infeasible" exclusion, distinct from a solver
"failed" non-convergence -- mirroring the split in_time_placebo already reports.

- _outer_solve_V_cv returns a 6th structural-infeasible flag; _placebo_fit_unit
  returns a (result, status) tuple with status in {ran, infeasible, failed}.
- in_space adds a public n_infeasible field; leave_one_out adds _loo_n_infeasible.
  Statuses gain all_placebos_infeasible / all_placebos_unusable (resp. all_refits_*);
  the per-row status column carries "infeasible"; DiagnosticReport surfaces reason_code
  + n_failed / n_infeasible for both surfaces.
- placebo_p_value / n_placebos are unchanged (both causes excluded from the rank /
  ATT range identically) -- only the diagnostic attribution is refined.
- __setstate__ backfills the new counters to 0 on legacy unpickle so summary() /
  to_dict() never AttributeError on a result pickled by an older version.

REGISTRY + CHANGELOG document the split; the TODO row is resolved.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@igerber igerber force-pushed the feature/sc-placebo-infeasible-reason-code branch from 83a7a3d to dcbde46 Compare July 2, 2026 13:38
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: dcbde46f95b1256f7066154e5f818dbbcc049a5b


Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected methods: SyntheticControl CV refit path, in-space placebo inference, ADH-2015 leave-one-out diagnostics, and shared _placebo_fit_unit().
  • The infeasible-vs-failed split is documented in the Methodology Registry at docs/methodology/REGISTRY.md:L2221-L2223, so it is not an undocumented methodology deviation.
  • The rank/range contract is preserved: both "failed" and "infeasible" refits are excluded from placebo_p_value / LOO ATT range.
  • Prior P3 items appear resolved: _require_placebo_reference() names the new statuses, and the LOO return docstring includes status="infeasible".
  • Focused tests could not be run because pytest is not installed in this environment.

Methodology

Finding: P3-informational — documented diagnostic-accounting refinement
Impact: The PR changes attribution of excluded CV refits, not the SCM estimand, donor weights, ADH-2010 rank formula, or LOO ATT range semantics. The Registry explicitly documents n_failed vs n_infeasible and states both are excluded identically from rank/range. Locations: docs/methodology/REGISTRY.md:L2190-L2193, docs/methodology/REGISTRY.md:L2221-L2223, diff_diff/synthetic_control_results.py:L949-L1064, diff_diff/synthetic_control_results.py:L1206-L1298.
Concrete fix: None required.

Code Quality

No findings.

Performance

No findings. The change threads status from existing refits and does not add extra solver runs.

Maintainability

No findings. Previous reason-map gap is addressed at diff_diff/synthetic_control_results.py:L1696-L1751.

Tech Debt

No findings. The prior TODO item for this exact split was removed from TODO.md, and the implemented behavior is now documented in the Registry.

Security

No findings.

Documentation/Tests

No findings. The previous LOO docstring gap is addressed at diff_diff/synthetic_control_results.py:L1107-L1118, and new tests cover all-infeasible/mixed statuses plus real CV structural infeasibility paths at tests/test_methodology_synthetic_control.py:L2254-L2407 and tests/test_methodology_synthetic_control.py:L3133-L3150.

Verification not run: pytest command failed with pytest: command not found.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 2, 2026
@igerber igerber merged commit afffd79 into main Jul 2, 2026
33 of 34 checks passed
@igerber igerber deleted the feature/sc-placebo-infeasible-reason-code branch July 2, 2026 16:39
@igerber igerber mentioned this pull request Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant