HAD fit(): extensive-margin warning + covariates= NotImplementedError

[igerber]

Summary

• HAD fit() overall-path extensive-margin UserWarning (TODO L74): fires when >= 10% of units have an exactly-zero post-period dose — a genuine untreated mass for which a standard DiD may be more appropriate. Closes paper-review checklist L191.
• HAD fit(covariates=...) raises NotImplementedError (TODO L73): an explicit keyword-only param pointing to the deferred Appendix B.1 / Theorem 6 covariate-adjusted extension, instead of a bare TypeError from an unknown kwarg.
• REGISTRY §HeterogeneousAdoptionDiD documents both as library Notes; new behavioral tests + deviation locks; retires the two TODO rows; CHANGELOG bullet. No change to any estimate or standard error.

Methodology references (required if estimator / math changes)

• Method name(s): HeterogeneousAdoptionDiD (de Chaisemartin, Ciccia, D'Haultfœuille & Knau 2026, arXiv:2405.04465)
• Paper / source link(s): Section 2 / Assumption 3 (extensive margin; "suggests running existing DiD" recommendation); Appendix B.1 / Theorem 6 (covariate-adjusted extension); Appendix B.2 (event-study never-treated requirement)
• Any intentional deviations from the source (and why): The 10% exactly-zero-dose warning cutoff is a LIBRARY CONVENTION — the paper prescribes "warn" with NO numeric threshold and explicitly retains small untreated shares (Garrett et al. 12/2954 ≈ 0.4%, nominal coverage), so the cutoff sits ~25× above that kept example. The warning is overall-path-only because the event-study path requires never-treated units per Appendix B.2. Documented as - **Note:** bullets in REGISTRY §HeterogeneousAdoptionDiD and locked in TestHADDeviations.

Validation

• Tests added/updated: tests/test_had.py (TestExtensiveMarginWarning + TestCovariatesTrap, 9 tests: fire / no-fire all-positive / just-below-10% boundary / both-aggregate trap / event-study-never-treated no-warn); tests/test_methodology_had.py::TestHADDeviations (2 deviation locks: the 10% convention incl. the pinned _HAD_EXTENSIVE_MARGIN_ZERO_DOSE_FRAC == 0.10 constant, and the covariates trap). Full HAD suite: 341 passed, 2 skipped. black/ruff/mypy clean.
• Backtest / simulation / notebook evidence (if applicable): N/A — fit-time UX only; no numerical-path change (so no SE/parity surface touched).

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

🤖 Generated with Claude Code

[github-actions]

Overall Assessment
⚠️ Needs changes. No methodology/estimation defect found, but the new public fit(covariates=...) parameter was not propagated to the locked LLM guide signature, which should fail CI.

Executive Summary

• Affected method: HeterogeneousAdoptionDiD.fit.
• Methodology deviations are documented in REGISTRY.md: the 10% extensive-margin warning cutoff and covariates= NotImplemented trap are P3 informational.
• No estimator math, weighting, variance, SE, or inference computation is changed.
• P1: diff_diff/guides/llms-full.txt omits the new covariates fit parameter, but tests/test_guides.py asserts the guide matches the real signature.
• I could not run pytest here because pytest is not installed.

Methodology

• Severity: P3 informational
  Impact: The 10% exactly-zero-dose warning cutoff is a library convention, not paper-prescribed, but it is explicitly documented as a **Note:** in the Methodology Registry. Overall-path-only placement matches the registry’s event-study rationale.
  Concrete fix: None required. See diff_diff/had.py:L3181-L3207 and docs/methodology/REGISTRY.md:L2932.

• Severity: P3 informational
  Impact: covariates= now raises NotImplementedError instead of TypeError; this matches the registry’s Appendix B.1 / Theorem 6 deferral.
  Concrete fix: None required. See diff_diff/had.py:L3027-L3045 and docs/methodology/REGISTRY.md:L2709-L2717, docs/methodology/REGISTRY.md:L2933.

Code Quality

• Severity: P1
  Impact: Public method signature propagation is incomplete. HeterogeneousAdoptionDiD.fit adds keyword-only covariates, but the HAD had.fit(...) block in llms-full.txt omits it. The existing guide test iterates over the real signature and asserts every parameter appears in that block, so this should fail on covariates.
  Concrete fix: Add covariates: Any | None = None or similar to diff_diff/guides/llms-full.txt after trends_lin, with a comment that non-None raises NotImplementedError for the deferred Appendix B.1 / Theorem 6 extension. Then run tests/test_guides.py::TestLLMSFullHADCoverage::test_llms_full_had_fit_signature_matches_real_api. See diff_diff/had.py:L2837-L2859, diff_diff/guides/llms-full.txt:L751-L766, and tests/test_guides.py:L383-L405.

Performance

• Severity: P3
  Impact: The new zero-dose warning adds one O(G) count on the overall path after aggregation. This is negligible relative to the estimator work.
  Concrete fix: None required.

Maintainability

• Severity: P2
  Impact: The registry now has the correct **Note:** for covariates=, but the requirements checklist still says the item is open and that current behavior is a bare TypeError. That contradicts the implemented behavior and the new note.
  Concrete fix: Update docs/methodology/REGISTRY.md:L2983 to [x] with a closed status matching this PR.

Tech Debt

• Severity: P3
  Impact: The PR removes the two TODO rows it implements. No new untracked silent-correctness debt identified.
  Concrete fix: None required.

Security

• Severity: P3
  Impact: No security-sensitive changes or secrets found in the diff.
  Concrete fix: None required.

Documentation/Tests

• Severity: P1
  Impact: Same guide-signature miss as above: docs and tests are inconsistent with the changed API.
  Concrete fix: Update diff_diff/guides/llms-full.txt and rerun the targeted guide test. I could not execute it locally because neither pytest nor python -m pytest is available in this environment.

Path to Approval

1. Add covariates to the HAD had.fit(...) signature block in diff_diff/guides/llms-full.txt, documenting that non-None raises NotImplementedError.
2. Run tests/test_guides.py::TestLLMSFullHADCoverage::test_llms_full_had_fit_signature_matches_real_api and the new HAD tests.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: deb1a745be6e7a19eda7b185b4ac39ab25b92e0d

---

Overall Assessment

✅ Looks good. The prior P1 guide-signature issue is resolved, and I found no unmitigated P0/P1 findings.

Executive Summary

• Affected method: HeterogeneousAdoptionDiD.fit.
• Prior P1 is fixed: covariates is now documented in the HAD LLM guide signature block, which satisfies the guide parity test. See diff_diff/guides/llms-full.txt:L751-L767 and tests/test_guides.py:L383-L405.
• The 10% extensive-margin warning is a documented library convention, so it is P3 informational under the review rules. See diff_diff/had.py:L3181-L3207 and docs/methodology/REGISTRY.md:L2932.
• The covariates= trap correctly raises NotImplementedError before either aggregate path can silently ignore it. See diff_diff/had.py:L3027-L3045.
• I could not run tests: this environment lacks pytest and numpy.

Methodology

• Severity: P3 informational
  Impact: The extensive-margin warning threshold and overall-only scope differ from the paper’s non-numeric “warn” recommendation, but this is explicitly documented as a **Note:** in the registry. No estimate, weighting, variance, or SE path is changed.
  Concrete fix: None required. References: diff_diff/had.py:L124-L133, diff_diff/had.py:L3181-L3207, docs/methodology/REGISTRY.md:L2932, docs/methodology/papers/dechaisemartin-2026-review.md:L119-L121.

• Severity: P3 informational
  Impact: covariates= remains deferred for Appendix B.1 / Theorem 6, and the new explicit NotImplementedError matches the registry and paper-review note rather than introducing an unsupported covariate-adjusted estimator.
  Concrete fix: None required. References: diff_diff/had.py:L2988-L2995, diff_diff/had.py:L3027-L3045, docs/methodology/REGISTRY.md:L2709-L2717, docs/methodology/REGISTRY.md:L2933.

Code Quality

• Severity: P3 informational
  Impact: No inline inference anti-pattern found in the changed HAD paths; overall and event-study inference still routes through safe_inference().
  Concrete fix: None required. References: diff_diff/had.py:L3682-L3691, diff_diff/had.py:L4576-L4584.

Performance

• Severity: P3 informational
  Impact: The new warning adds one count over d_arr on the overall path, which is negligible relative to estimator work.
  Concrete fix: None required. Reference: diff_diff/had.py:L3189-L3191.

Maintainability

• Severity: P3
  Impact: The registry deviation framing still says “Notes Add initial diff-diff library implementation #1-Add fixed effects and absorb parameters to DifferenceInDifferences #2” and “Add comprehensive code review for diff-diff library #5” even though this PR adds two more notes, so the prose undercounts the list. This is documentation hygiene only, not a methodology defect.
  Concrete fix: Update the framing sentence to avoid fixed note numbers or include the new notes. Reference: docs/methodology/REGISTRY.md:L2923-L2925.

Tech Debt

• Severity: P3 informational
  Impact: The two implemented HAD TODO rows were removed, and the registry checklist now marks the covariates= pointer closed. No untracked silent-correctness debt identified.
  Concrete fix: None required. References: TODO.md:L130-L143, docs/methodology/REGISTRY.md:L2983.

Security

• Severity: P3 informational
  Impact: No security-sensitive code paths, secrets, or external input execution changes found.
  Concrete fix: None required.

Documentation/Tests

• Severity: P3 informational
  Impact: The prior guide-signature miss is fixed, and new behavioral/deviation tests cover warning fire/no-fire boundaries, event-study no-warn scope, and covariates= rejection.
  Concrete fix: None required. References: diff_diff/guides/llms-full.txt:L751-L767, tests/test_had.py:L5660-L5770, tests/test_methodology_had.py:L1290-L1339.

• Severity: P3 informational
  Impact: Tests were not executable in this review environment because pytest and numpy are not installed.
  Concrete fix: Run the HAD and guide tests in CI or a dev environment with project dependencies installed.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: b04bf2bf73f907657db2125f5e2a7dca15a18c86

---

Overall Assessment

✅ Looks good. No unmitigated P0/P1 findings. The prior P1 guide-signature issue is resolved.

Executive Summary

• Affected method: HeterogeneousAdoptionDiD.fit.
• The 10% exactly-zero-dose warning is a documented library convention, so it is P3 informational, not a methodology defect. See diff_diff/had.py:L124-L133, diff_diff/had.py:L3181-L3207, docs/methodology/REGISTRY.md:L2932.
• covariates= now fails closed with NotImplementedError before either aggregate path can ignore it. See diff_diff/had.py:L3027-L3045.
• Inference paths still use safe_inference(); no new inline t-stat/p-value/CI anti-pattern found. See diff_diff/had.py:L3682-L3691, diff_diff/had.py:L4576-L4584.
• Could not run tests: pytest is not installed, and direct import also fails because numpy is missing.

Methodology

• Severity: P3 informational
  Impact: The extensive-margin warning threshold and overall-only scope differ from the paper’s non-numeric warning recommendation, but the deviation is explicitly documented as a **Note:** in the Methodology Registry. No estimate, weighting, variance, or SE computation changes.
  Concrete fix: None required. References: diff_diff/had.py:L3181-L3207, docs/methodology/REGISTRY.md:L2932, docs/methodology/papers/dechaisemartin-2026-review.md:L191.

• Severity: P3 informational
  Impact: covariates= remains deferred for Appendix B.1 / Theorem 6 and now raises a clear NotImplementedError, matching the registry’s future-work contract.
  Concrete fix: None required. References: docs/methodology/REGISTRY.md:L2709-L2717, docs/methodology/REGISTRY.md:L2933, diff_diff/had.py:L3027-L3045.

Code Quality

• Severity: P3 informational
  Impact: No new inline inference or partial-NaN guard pattern was introduced; affected inference still routes through safe_inference().
  Concrete fix: None required.

Performance

• Severity: P3 informational
  Impact: The added warning performs one zero-count over d_arr on the overall path; this is negligible relative to fitting.
  Concrete fix: None required. Reference: diff_diff/had.py:L3189-L3191.

Maintainability

• Severity: P3
  Impact: The registry framing still says “Notes Add initial diff-diff library implementation #1-Add fixed effects and absorb parameters to DifferenceInDifferences #2” / “Add comprehensive code review for diff-diff library #5” even though this PR adds more notes. This is documentation hygiene only.
  Concrete fix: Update the sentence to avoid fixed note numbering. Reference: docs/methodology/REGISTRY.md:L2923-L2925.

Tech Debt

• Severity: P3 informational
  Impact: The two implemented HAD TODO rows were removed, and the checklist now marks both items closed. No untracked correctness debt found.
  Concrete fix: None required. References: TODO.md:L137-L143, docs/methodology/REGISTRY.md:L2983.

Security

• Severity: P3 informational
  Impact: No security-sensitive changes, secrets, or external input execution paths found.
  Concrete fix: None required.

Documentation/Tests

• Severity: P3 informational
  Impact: The prior guide-signature gap is fixed: covariates is now documented in the HAD fit() guide block, and the guide parity test checks the real signature.
  Concrete fix: None required. References: diff_diff/guides/llms-full.txt:L751-L767, tests/test_guides.py:L383-L405.

• Severity: P3 informational
  Impact: Added tests cover warning fire/no-fire boundaries, event-study no-warn scope, and covariates= rejection.
  Concrete fix: None required. References: tests/test_had.py:L5660-L5770, tests/test_methodology_had.py:L1290-L1339.

• Severity: P3 informational
  Impact: Tests were not executable in this review environment because pytest and numpy are missing.
  Concrete fix: Run the focused HAD/methodology/guide tests in CI or a dev environment with project dependencies installed.