Skip to content

test(imputation): covariate-path R parity anchor vs didimputation#643

Open
igerber wants to merge 1 commit into
mainfrom
test/imputation-covariate-parity-anchor
Open

test(imputation): covariate-path R parity anchor vs didimputation#643
igerber wants to merge 1 commit into
mainfrom
test/imputation-covariate-parity-anchor

Conversation

@igerber

@igerber igerber commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • The covariate branch of ImputationDiD had no dedicated R anchor — only the no-covariate staggered panel was parity'd, though the covariate path shares the validated projection code. This adds the TODO row's "golden variant" (R available locally): a time-varying, unit-correlated covariate panel + didimputation reference values with first_stage = ~ 0 + x | unit + time (== diff-diff covariates=["x"]).
  • Generator discipline: the covariate scenario is appended after the base scenario in generate_didimputation_golden.R, so the base panel's RNG draws are unchanged — the committed base CSV and all base golden values reproduce byte-identically on regeneration (verified). The golden JSON gains a covariate block; a new didimputation_covariate_panel.csv is committed.
  • New TestImputationDiDCovariateParityR pins overall + per-horizon event-study ATT and SE. Observed agreement on the reference platform: SE ~2e-10 (the covariate-augmented untreated v_it projection + clustering machinery), ATT ~2e-7 (same platform class as the no-covariate anchor); asserted at abs=1e-6 (ATT) / abs=1e-7 (SE) for cross-platform robustness, with skip guards for missing fixtures.
  • REGISTRY v_it-derivation note updated to record the covariate anchor. Closes the imputation-validation TODO row.

Methodology references (required if estimator / math changes)

  • Method name(s): Borusyak, Jaravel & Spiess (2024) imputation DiD — covariate-augmented first stage (no estimator code changed; golden fixture + tests + REGISTRY note only)
  • Paper / source link(s): REGISTRY.md § ImputationDiD ("Note on v_it derivation", updated); R didimputation 0.5.0 as the parity reference (R 4.5.2)
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: tests/test_methodology_imputation.py::TestImputationDiDCovariateParityR (4 tests; full file 48 pass).
  • Evidence: regeneration reproduces all pre-existing base panel bytes and golden values identically; the new block's values were cross-checked live against the Python fit before pinning (SE diffs 1e-11..2e-10 across overall + 6 horizons).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@igerber igerber force-pushed the test/imputation-covariate-parity-anchor branch from 8ee3bf9 to 2b9a7db Compare July 7, 2026 21:04
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 2b9a7dba8f3f81b8b178f58b7ab646d0993acccd


Overall Assessment

✅ Looks good

Executive Summary

  • No estimator code, math, weighting, variance/SE, or defaults were changed.
  • Affected method: ImputationDiD, specifically Borusyak-Jaravel-Spiess imputation DiD covariate first-stage parity.
  • The new R anchor matches the documented registry contract for covariate-augmented v_it projection in docs/methodology/REGISTRY.md:L1556-L1560.
  • The TODO row for the missing covariate-path parity anchor is appropriately closed.
  • I could not run the Python parity fit locally because this review environment lacks pandas; static fixture/registry/test review found no blocking issues.

Methodology

No findings.

  • Severity: None
  • Impact: The PR adds parity coverage only. The documented method remains ImputationDiD / BJS 2024; the covariate first stage y ~ x | unit + time is aligned with the registry’s covariate projection description.
  • Concrete fix: None.

Code Quality

No findings.

  • Severity: None
  • Impact: The new fixture path and helper _fit() are scoped to the new parity class and follow the existing R-parity test pattern in tests/test_methodology_imputation.py:L1176-L1250.
  • Concrete fix: None.

Performance

No findings.

  • Severity: None
  • Impact: The added CSV fixture is small for test data, and the new tests run only the targeted parity fits.
  • Concrete fix: None.

Maintainability

No findings.

  • Severity: None
  • Impact: The generator appends the covariate scenario after the base scenario, preserving existing RNG output for the base fixture; the registry note records the new anchor.
  • Concrete fix: None.

Tech Debt

No findings.

  • Severity: None
  • Impact: The existing TODO entry for missing ImputationDiD covariate-path variance parity is removed because this PR adds the tracked R anchor.
  • Concrete fix: None.

Security

No findings.

  • Severity: None
  • Impact: No secrets or sensitive data patterns are introduced; the new fixture is synthetic benchmark data.
  • Concrete fix: None.

Documentation/Tests

No PR findings.

  • Severity: P3-informational
  • Impact: I could not execute the new tests locally because pandas is not installed in the review environment. Static checks confirm the golden JSON covariate block is finite and length-aligned, and the CSV fixture has 1,440 rows across 180 units and 8 periods.
  • Concrete fix: None required for the PR; CI should run tests/test_methodology_imputation.py::TestImputationDiDCovariateParityR.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 7, 2026
@igerber igerber force-pushed the test/imputation-covariate-parity-anchor branch from 2b9a7db to 876602b Compare July 7, 2026 23:20
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 876602b339b15dc1a59ea3a19b6aaaf926ea0868


Overall Assessment

✅ Looks good

Executive Summary

  • No estimator code, math, defaults, weighting, or inference implementation changed.
  • Affected method: ImputationDiD, specifically BJS imputation DiD with covariates in the untreated first-stage model.
  • The new R parity anchor matches the registry contract for covariate-augmented v_it projection and Theorem 3 variance.
  • The prior TODO item for missing covariate-path variance parity is closed by the new fixture and tests.
  • I could not run tests locally because this environment lacks pytest and pandas; static and fixture checks found no blockers.

Methodology

No findings.

  • Severity: None
  • Impact: docs/methodology/REGISTRY.md:L1555-L1566 documents the BJS Theorem 3 variance and exact untreated projection for FE-only and covariate cases. The generator uses R didimputation with first_stage = ~ 0 + x | unit + time at benchmarks/R/generate_didimputation_golden.R:L150-L163, matching the test’s covariates=["x"] call at tests/test_methodology_imputation.py:L1208-L1218.
  • Concrete fix: None.

Code Quality

No findings.

  • Severity: None
  • Impact: The new fixture and _fit() helper are scoped to TestImputationDiDCovariateParityR, with skip guards for missing JSON/CSV fixtures at tests/test_methodology_imputation.py:L1176-L1190.
  • Concrete fix: None.

Performance

No findings.

  • Severity: None
  • Impact: The added CSV fixture is small synthetic test data, and the new tests run targeted overall/event-study parity fits only.
  • Concrete fix: None.

Maintainability

No findings.

  • Severity: None
  • Impact: The R generator appends the covariate scenario after the base scenario at benchmarks/R/generate_didimputation_golden.R:L115-L178, preserving the existing base RNG stream before adding the covariate golden block.
  • Concrete fix: None.

Tech Debt

No findings.

  • Severity: None
  • Impact: The removed TODO row was specifically addressed by the new R covariate anchor and registry note.
  • Concrete fix: None.

Security

No findings.

  • Severity: None
  • Impact: The new data fixture is synthetic numeric panel data; no secrets or sensitive data patterns are introduced.
  • Concrete fix: None.

Documentation/Tests

P3-informational.

  • Severity: P3
  • Impact: The new golden block is finite and length-aligned at benchmarks/data/didimputation_golden.json:L22-L33; the CSV fixture has 1,440 rows across 180 units and 8 periods. I could not execute the parity tests because pytest and pandas are unavailable in this environment.
  • Concrete fix: None required for this PR; CI should run tests/test_methodology_imputation.py::TestImputationDiDCovariateParityR.

The covariate branch of ImputationDiD (first-stage imputation model
y ~ x | unit + time on the untreated sample; R first_stage =
~ 0 + x | unit + time == diff-diff covariates=['x']) had no dedicated
R anchor — only the no-covariate staggered panel was parity'd. Adds a
time-varying, unit-correlated covariate panel to the didimputation
golden generator (appended after the base scenario so its RNG draws —
and the committed base panel + golden values — reproduce
byte-identically; verified) and TestImputationDiDCovariateParityR
pinning overall + per-horizon event-study ATT and SE. Observed
agreement: SE ~2e-10, ATT ~2e-7 on the reference platform; asserted at
abs=1e-6/1e-7 for cross-platform robustness. REGISTRY v_it-derivation
note updated with the covariate anchor.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@igerber igerber force-pushed the test/imputation-covariate-parity-anchor branch from 876602b to e23889f Compare July 8, 2026 02:22
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: e23889fa69b1da64cf38cebfac1e36736ba083b0


Overall Assessment

✅ Looks good

Executive Summary

  • No estimator implementation, default behavior, weighting, or inference code changed.
  • Affected method: ImputationDiD, specifically the BJS imputation DiD covariate first-stage path.
  • The new R generator uses first_stage = ~ 0 + x | unit + time, matching the Python test call with covariates=["x"].
  • The registry now documents the separate covariate-path v_it R anchor, and the prior TODO coverage gap is removed.
  • Static fixture checks passed: covariate panel has 1,440 rows, finite y/x, 180 units, 8 periods, and aligned 6-horizon ATT/SE golden arrays.
  • I could not execute tests locally because pytest is unavailable in this environment.

Methodology

No findings.

  • Severity: None
  • Impact: The registry’s BJS Theorem 3 contract still requires the exact untreated projection for FE-only and covariate cases, and the PR’s added note records the covariate R anchor without changing methodology. See docs/methodology/REGISTRY.md:L1555-L1566.
  • Concrete fix: None.

Code Quality

No findings.

  • Severity: None
  • Impact: The added test fixture and helper are scoped to TestImputationDiDCovariateParityR; the Python call exercises covariates=["x"] directly. See tests/test_methodology_imputation.py:L1176-L1250.
  • Concrete fix: None.

Performance

No findings.

  • Severity: None
  • Impact: Runtime impact is limited to a small committed fixture and four targeted parity tests. The generator appends the covariate scenario after the base scenario, preserving the base RNG stream. See benchmarks/R/generate_didimputation_golden.R:L115-L178.
  • Concrete fix: None.

Maintainability

No findings.

  • Severity: None
  • Impact: The golden JSON keeps the base block intact and adds a separate covariate block with explicit panel and first-stage metadata. See benchmarks/data/didimputation_golden.json:L22-L33.
  • Concrete fix: None.

Tech Debt

No findings.

  • Severity: None
  • Impact: The removed TODO row is addressed by the new covariate R parity anchor and registry note; no new untracked limitation is introduced.
  • Concrete fix: None.

Security

No findings.

  • Severity: None
  • Impact: The new fixture is synthetic numeric panel data. No secret-like material was found in the changed scope relevant to this PR.
  • Concrete fix: None.

Documentation/Tests

P3-informational.

  • Severity: P3
  • Impact: I could not run tests/test_methodology_imputation.py::TestImputationDiDCovariateParityR because pytest is not installed (pytest: command not found; python -m pytest: No module named pytest). Static checks of the fixture and golden arrays found no issue.
  • Concrete fix: None required for this PR; CI should run the targeted parity class.

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