Skip to content

test(twfe): tighten live-R fixest parity bands to measured precision (SE-audit C7/C8)#663

Open
igerber wants to merge 2 commits into
mainfrom
test/twfe-live-r-parity-tightening
Open

test(twfe): tighten live-R fixest parity bands to measured precision (SE-audit C7/C8)#663
igerber wants to merge 2 commits into
mainfrom
test/twfe-live-r-parity-tightening

Conversation

@igerber

@igerber igerber commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Continues the SE-audit C7/C8 re-derivation from a fresh loose-tolerance inventory. TestRBenchmarkTWFE's live-R (fixest) parity bands predate the D4 within-transform rescale; measured against fixest 0.14.2: ATT agrees at ~4e-16 relative (asserted 0.1% → now rtol=1e-12), the cluster-robust SE and CI bands drop 0.01 → 0.005, pinning the documented ~0.25% absorbed-FE fixest-CR1 ssc deviation instead of sitting 4x above it.
  • The p-value assert (atol=0.01) was vacuous — and its investigation root-caused a mislabeled deviation: both sides use t-distributions, but Python's clustered CR1 inference uses the residual df (t(148) on this panel) while fixest uses the cluster df G−1 (t(49)); at |t|≈22 the tails diverge by ~21 orders (~1e-48 vs ~4e-27). This inference-df convention deviation is now documented as a REGISTRY **Note (deviation from R):** in the TwoWayFixedEffects section, the test becomes an honest both-are-zero pin at atol=1e-12, and a new Actionable TODO row poses the library-wide decision (adopt cluster-df — the common applied recommendation — vs keep residual-df as documented; adopting moves every clustered p-value/CI, so it needs its own impact sweep).
  • Docstrings record the measured gaps and attribute each band to its deviation source (the CI band inherits both the SE scale band and the df-convention critical-value difference). Remaining loose-tolerance inventory candidates were triaged as statistical-by-design (DGP recovery, RNG moment checks) or fixture-not-committed (triplediff CSVs) — recorded in the SE-audit TODO row.
  • Local review: 3 rounds (round-1 P1 correctly rejected my initial "normal-vs-t" attribution → empirical df identification; rounds 2-3 doc precision), round 3 fully informational.

Methodology references (required if estimator / math changes)

  • Method name(s): TWFE CR1 clustered inference — test tolerances + deviation documentation only, no code change
  • Paper / source link(s): REGISTRY.md §TwoWayFixedEffects (new inference-df deviation Note; existing D4/ssc notes)
  • Any intentional deviations from the source (and why): the residual-df vs cluster-df convention is now explicitly documented (decision on adopting fixest/Stata's G−1 tracked as its own TODO row).

Validation

  • Tests added/updated: tests/test_methodology_twfe.py::TestRBenchmarkTWFE (6 assertions tightened, docstrings rewritten). All 6 pass against live R.
  • Backtest / simulation / notebook evidence (if applicable): in-session gap measurements via the class's own fixtures (values quoted in docstrings).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected method: TwoWayFixedEffects clustered CR1 analytical inference parity against fixest.
  • No estimator/math/default behavior changed; this PR changes docs, TODO/changelog, and test tolerances.
  • The residual-df vs fixest cluster-df inference convention is explicitly documented as a Note (deviation from R) in docs/methodology/REGISTRY.md:L361-L372, so it is not a defect under the review rules.
  • Code cross-check matches the documentation: TWFE sets absorbed-FE df adjustment in diff_diff/twfe.py:L407-L413; LinearRegression stores residual df and routes inference through safe_inference() in diff_diff/linalg.py:L4126-L4375.
  • I did not run the live-R tests in this review.

Methodology

  • Severity: P3 informational
    Impact: tests/test_methodology_twfe.py:L490-L508 now pins p-values with atol=1e-12, but because both Python and fixest p-values are far below that threshold, this is a “both numerically zero” check rather than a discriminating test of residual-df vs cluster-df behavior. This is acceptable because the deviation is documented in REGISTRY.md and tracked in TODO.md:L60.
    Concrete fix: No required fix. If the project later wants a df-convention regression test, add a moderate-|t| fixture where t(148) and t(49) p-values/CIs differ above tolerance.

Code Quality

  • Severity: None
    Impact: No modified production code paths or new inline inference anti-patterns found.
    Concrete fix: None.

Performance

  • Severity: None
    Impact: Test tolerance and documentation changes only.
    Concrete fix: None.

Maintainability

  • Severity: None
    Impact: The new deviation note and TODO row make the convention explicit and discoverable.
    Concrete fix: None.

Tech Debt

  • Severity: P3 informational
    Impact: The cluster-df convention decision is properly tracked in TODO.md:L60, so it is deferrable and non-blocking.
    Concrete fix: None required for this PR.

Security

  • Severity: None
    Impact: No secrets or security-sensitive changes observed in the changed files.
    Concrete fix: None.

Documentation/Tests

  • Severity: None
    Impact: Registry, changelog, TODO, and test docstrings are aligned with the tightened TWFE live-R parity bands and documented deviation.
    Concrete fix: None.

@igerber igerber force-pushed the test/twfe-live-r-parity-tightening branch from 58e9d00 to 5e27a9b Compare July 9, 2026 12:37
…(SE-audit C7/C8 re-derivation)

TestRBenchmarkTWFE's assertions predate the D4 within-transform
rescale. Measured against live R (fixest 0.14.2): ATT agrees at ~4e-16
relative (asserted 0.1% -> rtol=1e-12); the cluster-robust SE and CI
bands drop 0.01 -> 0.005, pinning the documented ~0.25% absorbed-FE
fixest-CR1 DOF-convention deviation instead of sitting 4x above it;
the p-value assert (atol=0.01 was vacuous: at |t|~15 Python's normal
tail is ~1e-49 vs fixest's t-tail ~1e-27, the documented distribution
deviation) becomes a both-are-zero pin at atol=1e-12. Docstrings
record measured gaps + deviation attributions. TODO SE-audit row
updated with the re-derivation findings (remaining inventory
candidates are statistical-by-design or fixture-not-committed).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@igerber igerber force-pushed the test/twfe-live-r-parity-tightening branch from 5e27a9b to a08bc0e Compare July 9, 2026 12:38
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: a08bc0e66022e84a8feeee6164e3691b5a07132d


Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected method: TwoWayFixedEffects analytical clustered CR1 inference parity against R fixest.
  • No estimator, variance, weighting, or default behavior changed; this PR changes documentation, TODO tracking, changelog text, and test tolerances.
  • The residual-df vs cluster-df convention is documented with a Registry Note (deviation from R), so it is not a methodology defect under the review rules.
  • The implementation matches the documentation: TWFE passes absorbed-FE df adjustment, and inference uses fitted residual df via safe_inference().
  • I could not run the modified pytest class because pytest is not installed in this runner.

Methodology

  • Severity: P3 informational
    Impact: Python clustered CR1 inference uses residual df while fixest uses cluster df G - 1; this can move p-values/CIs, especially in tail cases. The deviation is explicitly documented in docs/methodology/REGISTRY.md:L361-L372 and tracked in TODO.md:L59, so it is not a blocker. Implementation evidence matches: diff_diff/twfe.py:L413, diff_diff/linalg.py:L4126, diff_diff/linalg.py:L4315-L4375.
    Concrete fix: No required fix. A future behavior change should be handled as its own library-wide df-convention PR.

Code Quality

  • Severity: None
    Impact: No modified production code and no new inline inference anti-pattern in the changed test path. Inference remains routed through safe_inference() at diff_diff/linalg.py:L4372-L4375, with all-or-nothing NaN gating in diff_diff/utils.py:L327-L366.
    Concrete fix: None.

Performance

  • Severity: None
    Impact: Documentation and test tolerance changes only.
    Concrete fix: None.

Maintainability

  • Severity: None
    Impact: The Registry note and TODO row make the convention discoverable rather than implicit.
    Concrete fix: None.

Tech Debt

  • Severity: P3 informational
    Impact: The unresolved choice to adopt cluster df or keep residual df is properly tracked in TODO.md:L58-L59 as actionable deferred work.
    Concrete fix: None required for this PR.

Security

  • Severity: None
    Impact: No security-sensitive code paths or secrets observed in the changed files.
    Concrete fix: None.

Documentation/Tests

  • Severity: P3 informational
    Impact: tests/test_methodology_twfe.py:L490-L508 now pins p-values at atol=1e-12, but this is still a “both numerically zero” check, not a discriminating df-convention regression. Acceptable because the deviation is documented and tracked.
    Concrete fix: Optional only: add a moderate-|t| fixture where t(148) and t(49) p-values/CIs differ above tolerance. Verification not run: pytest was unavailable.

… |t|

Review optional P3: the R-benchmark p-value pin sits at |t|~22 where the
residual-df and cluster-df (G-1) tails are both numerically zero and
cannot distinguish the conventions. New fixture tuned to |t|~1.8 (t(148)
vs t(49) p-values differ ~5% relative): asserts the estimator's p-value
matches the residual-df tail to 1e-10 AND differs measurably from the
cluster-df tail, with a regime guard on |t| so data-gen drift cannot
silently defang the assertions. REGISTRY deviation note now cites it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lbd6nqWmg4snvvBmegwqiw
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: b1ab9e6fd1a0c42fcebcf04ed5c9f23527e05646


Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected method: TwoWayFixedEffects analytical clustered CR1 inference parity against R fixest.
  • No production estimator, variance, weighting, identification, or default behavior changed.
  • The residual-df vs cluster-df convention is now documented with a Registry Note (deviation from R), so it is not a methodology defect.
  • The new moderate-|t| test makes the p-value df convention distinguishable instead of relying only on “both are zero” live-R tails.
  • I could not execute tests in this runner: pytest and numpy are not installed.

Methodology

  • Severity: P3 informational
    Impact: Python clustered CR1 inference uses residual df (n - K_full) while fixest/Stata use cluster df (G - 1). This affects clustered p-values/CIs, but the deviation is explicitly documented in docs/methodology/REGISTRY.md:L361-L374 and tracked in TODO.md:L58-L59. The implementation matches that documentation: TWFE passes absorbed-FE df adjustment at diff_diff/twfe.py:L413, LinearRegression stores residual df at diff_diff/linalg.py:L4126, and inference uses that df through safe_inference() at diff_diff/linalg.py:L4315-L4375.
    Concrete fix: No required fix for this PR. Any move to cluster df should be a separate library-wide behavior-change PR.

Code Quality

  • Severity: None
    Impact: No new production inline inference anti-pattern was introduced. Analytical inference still routes through safe_inference() with all-or-nothing NaN gating at diff_diff/utils.py:L327-L366.
    Concrete fix: None.

Performance

  • Severity: None
    Impact: Documentation and test-only changes; no runtime path changed.
    Concrete fix: None.

Maintainability

  • Severity: None
    Impact: The Registry note and TODO row make the df convention explicit and discoverable.
    Concrete fix: None.

Tech Debt

  • Severity: P3 informational
    Impact: The unresolved choice to keep residual df or adopt cluster df is properly tracked as actionable deferred work in TODO.md:L59.
    Concrete fix: None required in this PR.

Security

  • Severity: None
    Impact: No security-sensitive code paths or secrets observed in the changed files.
    Concrete fix: None.

Documentation/Tests

  • Severity: P3 informational
    Impact: tests/test_methodology_twfe.py:L511-L574 adds a moderate-|t| fixture that distinguishes residual df from cluster df, addressing the prior concern that test_pvalue_matches_r_twfe only pins both tails near zero. Verification not run because this runner lacks pytest and numpy.
    Concrete fix: None required; rely on CI/live-R environment to execute the updated class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant