feat: dedicated practitioner_next_steps() handler for ChangesInChanges/QDiD

[igerber]

Summary

• Replace the _handle_generic fallback for ChangesInChangesResults (shared by CiC and QDiD via the QDiDResults alias) with a dedicated handler branching on the estimator field and covariate status - the generic fallback recommended mean parallel-trends checks and HonestDiD, neither of which fits a distributional 2x2 estimator with no event-study effects.
• Step 2 (assumptions) is overridden for this results type: distributional assumptions (Athey-Imbens monotone outcome model, time-invariance of U, support; QDiD's stronger four-cell restriction) instead of the generic parallel-trends-variant text; Step 3 frames check_parallel_trends() as a necessary-but-insufficient means screen.
• Branch-specific guidance: eq. 17 interior range (unconditional CiC only), conditional-envelope support (covariate CiC only - the QDiD covariate path has no support diagnostic), the p. 447 CiC-over-QDiD recommendation with the footnote-21 monotonicity caveat, two-pre-period distributional placebo, sup-t uniform-band reading of the QTE profile, with/without-covariates comparison, and mean-DiD anchoring (population-equivalence claim scoped to unconditional QDiD only; covariate QDiD gets a descriptive covariate-adjusted anchor).
• Refit snippets preserve the fit's specification via _cic_fit_snippet/_did_anchor_snippet: panel=True + unit= (bootstrap scheme) and the actual covariate list are mirrored; inference settings are normalized to defaults and the snippet says so.
• Bootstrap-health warnings (_cic_bootstrap_warnings): inference disabled at n_bootstrap=0; replicate-failure share above the 5% fit-time materiality threshold (mirrors warn_bootstrap_failure_rate). Per-instance display names for the shared results class ("ChangesInChanges (CiC)" vs "QDiD").
• Housekeeping: TODO practitioner-handler row removed, CHANGELOG [Unreleased] bullet, docs/doc-deps.yaml practitioner note extended.

Methodology references (required if estimator / math changes)

• Method name(s): ChangesInChanges (CiC), QDiD - practitioner guidance only; no estimation or inference behavior touched.
• Paper / source link(s): Athey & Imbens (2006), Econometrica 74(2) (Assumptions 3.1-3.4, eq. 17, Corollary 3.1/Theorem 5.3, Section 4 bounds, footnote 21, pp. 447/476); Melly & Santangelo (2015) Assumption 4 (conditional-envelope diagnostic wording); Baker et al. (2025) 8-step framework.
• Any intentional deviations from the source (and why): None - the handler restates the documented fit-time diagnostics and REGISTRY notes (interior range, envelope scope, QDiD monotonicity check scope, fixed-95% sup-t bands, bootstrap-only inference) without altering them.

Validation

• Tests added/updated: tests/test_practitioner.py - 30 new tests (TestCiCHandler, TestCiCHandlerSpecificationPropagation): dispatch + per-instance display names, branch-content locks (interior-range/envelope/prefer-CiC step scoping), bare-mock unknown-kind default lock, snippet ast.parse sweeps plus semantic assertions that snippets mirror covariates/panel/unit (incl. a panel+covariates combined fixture), bootstrap-health warning paths (n_bootstrap=0, 50% failure, 2% below-threshold negative), NaN-ATT warning, completed_steps filtering. Fixture fits verified warning-free under warnings.simplefilter('error').
• Backtest / simulation / notebook evidence (if applicable): N/A - guidance strings only.

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

PR Review: feat dedicated practitioner_next_steps() handler for ChangesInChanges/QDiD

Overall Assessment

⚠️ Needs changes — one unmitigated P1 methodology-guidance issue.

Executive Summary

• The PR is guidance-only; I did not find estimator, weighting, variance, or stored inference changes.
• The dedicated CiC/QDiD dispatch mostly matches the registry and in-code source contracts.
• P1: The CiC branch incorrectly frames check_parallel_trends() on pre-period means as a necessary screen. CiC does not require mean parallel trends.
• Minor bootstrap/snippet wording issues are informational and do not affect statistical output.
• Tests were not run; the local environment is missing numpy.

Methodology

Finding 1

Severity: P1
Location: diff_diff/practitioner.py:L1527-L1559
Impact: The CiC branch tells users that a pre-period mean-trend break “undermines” CiC and labels check_parallel_trends() on means as “necessary-but-insufficient.” That is too strong for CiC. The registry/source contract is monotone outcome model, within-group time-invariance of U, and support inclusion; CiC can be valid under nonlinear transformations where mean trends are not parallel. This could lead practitioners to reject valid CiC analyses for a mean-DiD reason not required by the method. See docs/methodology/REGISTRY.md:L3857-L3865 and docs/methodology/papers/athey-imbens-2006-review.md:L35-L48.
Concrete fix: Split the Step 3 text by estimator. For CiC, reword the mean-trends check as an optional descriptive mean-DiD anchor, not a necessary screen, and point users to the two-pre-period distributional placebo/refit as the relevant falsification exercise. Keep stronger mean-screen language only for QDiD if desired, since QDiD’s population mean equivalence to standard DiD is separately documented.

Code Quality

Finding 2

Severity: P3
Location: diff_diff/practitioner.py:L2016-L2024
Impact: _cic_bootstrap_warnings() only accepts Python int/float for n_bootstrap, while ChangesInChanges accepts np.integer. It also misses n_bootstrap=1, n_bootstrap_valid=1, where inference is NaN because the estimator requires at least 2 valid replicates. This is a diagnostic gap only; the estimator output remains NaN-safe.
Concrete fix: Accept numbers.Integral/np.integer, and warn when 0 < n_bootstrap < 2 or n_bootstrap_valid < max(2, 0.5 * n_bootstrap).

Performance

No findings.

Maintainability

No findings.

Tech Debt

No findings. The removed TODO row is appropriate because the handler is implemented in this PR.

Security

No findings.

Documentation/Tests

Finding 3

Severity: P3
Location: diff_diff/practitioner.py:L1447-L1451, diff_diff/practitioner.py:L1475, tests/test_practitioner.py:L1504-L1510
Impact: The snippet says n_bootstrap/seed shown are defaults, but seed=42 is not a constructor default; the default is seed=None.
Concrete fix: Change the comment/test to say n_bootstrap shown is the default; seed=42 is illustrative for reproducibility.

Path To Approval

1. Reword the CiC Step 3 guidance so mean parallel-trends checks are not described as necessary for CiC.
2. Add/update a test asserting the CiC branch does not contain “necessary-but-insufficient MEANS screen” or equivalent mean-PT-as-required wording.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: a222bfe8b2c5b387df7a5ea98b09640c232801dd

---

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• The prior P1 is resolved: CiC guidance no longer treats mean parallel trends as a necessary screen.
• The CiC/QDiD branch wording matches the methodology registry and in-code contracts for assumptions, support diagnostics, QDiD monotonicity scope, bootstrap-only inference, and fixed-95% uniform bands.
• Previous P3 issues are addressed: seed=42 is now labeled illustrative, and bootstrap warnings handle n_bootstrap=1 plus NumPy scalar values.
• No estimator, weighting, variance, or stored inference behavior is changed.
• I could not run tests locally because pytest is not installed in this environment.

Methodology

No findings.

The new CiC Step 3 correctly says mean parallel trends are not required and a mean-trend break is not by itself evidence against CiC (diff_diff/practitioner.py:L1546-L1574). This aligns with the registry’s CiC assumptions: monotone outcome model, within-group time-invariance of U, and support inclusion, not mean parallel trends.

QDiD guidance is also scoped appropriately: stronger all-four-cells unobservable restriction, p. 447 CiC-over-QDiD recommendation, unconditional-only population mean equivalence, and covariate QDiD as descriptive mean-DiD anchoring only (diff_diff/practitioner.py:L1516-L1544, diff_diff/practitioner.py:L1826-L1877).

Code Quality

No findings.

The dedicated handler dispatch and per-instance display name are narrowly scoped and defensive for mock/missing estimator fields (diff_diff/practitioner.py:L52-L66, diff_diff/practitioner.py:L1957-L1975).

Performance

No findings.

The change is guidance-only. Added tests include covariate CiC/QDiD fits with n_bootstrap=0 where appropriate, avoiding bootstrap QR cost.

Maintainability

No findings.

The handler keeps branch-specific text in one place and adds targeted tests for dispatch, branch scope, snippets, and prior-review locks (tests/test_practitioner.py:L1230-L1552).

Tech Debt

No findings.

Removing the TODO row for the dedicated practitioner handler is appropriate because the handler is implemented in this PR (TODO.md:L48-L55).

Security

No findings.

No secrets or risky external inputs were introduced.

Documentation/Tests

No PR findings.

Validation note: I attempted PYTHONDONTWRITEBYTECODE=1 pytest -q tests/test_practitioner.py -k 'CiCHandler' -p no:cacheprovider, but the environment returned pytest: command not found. The new tests do cover the prior P1 wording lock (tests/test_practitioner.py:L1514-L1523), seed wording (tests/test_practitioner.py:L1504-L1512), and bootstrap warning edge cases (tests/test_practitioner.py:L1533-L1552).