bench: FE-absorption benchmark lane with BEFORE baselines + pyfixest yardstick (scenarios 7-13)

[igerber]

Summary

• Adds the FE-absorption benchmark suite (benchmarks/speed_review/bench_fe_absorption.py, scenarios 7-13 in docs/performance-scenarios.md): seven realistic workloads timing the MAP-demeaning hot path (demean_by_groups / within_transform) - county policy event study (SunAbraham, 3,109 counties x 60 months), firm panel with entry/exit churn (SunAbraham, 2.4M rows), scanner store-week (TWFE, 3.3M rows), 5M-order geo experiment (DiD absorb=), survey BRR replicates (DiD absorb + 80 replicate re-demeans), correlated-FE stress, and a small-panel regression guard.
• Noise protocol per the measurement plan: every (scenario, repeat) in a fresh subprocess, strictly sequential, fixed seeds; pooled median/min/max/CV with CV > 10% flagged as unusable. Committed BEFORE baselines (baselines/fe_absorption_before.json) are at CV 0.3-8.6%.
• ATT/SE identity capture + --check-estimates gate so the follow-up optimization PR can prove estimates are unchanged (unified tolerances recorded in the JSON; gate validated in both pass and fail directions, exit 0/1).
• Optional pyfixest yardstick lane (bench_fe_absorption_pyfixest.py), guarded on import pyfixest (exits 0 with a message when absent - never a dependency). Asserts coefficient parity < 1e-6 vs the diff-diff baseline on the four exact-estimand scenarios (measured 6e-13 to 2.7e-9); Sun-Abraham scenarios are timing-only via the saturated i(rel_time) regression (pyfixest 0.60 has no sunab()); the survey scenario is skipped (no BRR path in pyfixest).
• Docs: extends docs/performance-scenarios.md with the scenario definitions + provenance; adds a findings section to docs/performance-plan.md with an auto-generated results table (fe_absorption marker pair registered in gen_findings_tables.py); README + CHANGELOG entries.
• The committed BEFORE baseline records the tail_stress non-convergence warning (demean_by_groups ... did not converge in 100 iterations) as evidence of the max_iter=100 contract issue the follow-up PR addresses.

Methodology references (required if estimator / math changes)

• Method name(s): N/A - no methodology changes (measurement-only; no diff_diff/ source files touched)
• Paper / source link(s): N/A
• Any intentional deviations from the source (and why): None

Validation

• Tests added/updated: No test changes (benchmarks are not pytest-collected; testpaths=["tests"] verified, 8,240 tests still collect cleanly)
• Backtest / simulation / notebook evidence (if applicable): Full suite ran end-to-end on Apple Silicon M4 (baselines committed); yardstick lane ran both with and without pyfixest installed to prove the guard; identity gate negative-tested with a perturbed baseline (exit 1)

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

Overall Assessment

✅ Looks good. The PR is measurement-only and does not change estimator math, variance, weighting, assumptions, or defaults. I found no unmitigated P0/P1 methodology or inference issues.

Executive Summary

• No diff_diff/ estimator source changes; affected methods are benchmarked only: SunAbraham, TwoWayFixedEffects, and DifferenceInDifferences(absorb=...).
• Registry contracts for FWL/within-transform, MAP non-convergence warnings, and BRR replicate variance are respected.
• P2: pyfixest CVs above 10% are not flagged in the generated docs table, despite the documented noise protocol.
• P2: Sun-Abraham pyfixest rows are timing-only proxies, but the results table presents them beside exact-estimand rows without a visible caveat.
• No inline inference anti-patterns, secrets, or security concerns found in changed code.

Methodology

No blocking findings.

Affected methods: SunAbraham, TwoWayFixedEffects, and DifferenceInDifferences(absorb=..., survey_design=...) are invoked by benchmarks only at benchmarks/speed_review/bench_fe_absorption.py:L82-L116. The registry documents TWFE within-transformation/FWL requirements at docs/methodology/REGISTRY.md:L333-L356, DiD absorb MAP residualization at docs/methodology/REGISTRY.md:L4301-L4314, MAP non-convergence warning behavior at docs/methodology/REGISTRY.md:L4315-L4324, and BRR replicate formulas at docs/methodology/REGISTRY.md:L4463-L4511. The PR does not alter those implementations.

P3 informational: tail_stress intentionally captures the documented max_iter=100 non-convergence warning. This is consistent with the registry warning contract and is not a defect.

Code Quality

No findings. The benchmark code is isolated, deterministic by seeded DGPs, and does not introduce public API surface.

Performance

Finding 1
Severity: P2
Impact: docs/performance-plan.md reports pyfixest results with CVs above the documented 10% unusable threshold, but gen_findings_tables.py only appends (noisy) when a record has a noisy field. The pyfixest JSON has fit_cv only, so rows like firm churn at 20.3% and scanner TWFE at 12.3% appear unflagged. See benchmarks/speed_review/gen_findings_tables.py:L231-L235 and docs/performance-plan.md:L56-L59.
Concrete fix: In cell(rec), compute noise from either rec.get("noisy") or rec["fit_cv"] > 0.10, and regenerate the table.

Finding 2
Severity: P2
Impact: The pyfixest Sun-Abraham rows are timing-only and use i(rel_time) rather than the library’s cohort × relative-time interaction design. The script documents this caveat at benchmarks/speed_review/bench_fe_absorption_pyfixest.py:L14-L18, while the local SunAbraham implementation builds cohort × relative-time interactions at diff_diff/sun_abraham.py:L1350-L1352 and diff_diff/sun_abraham.py:L1396-L1406. The docs table at docs/performance-plan.md:L56-L57 does not surface that these two pyfixest cells are not exact-estimand or exact-column-load comparisons.
Concrete fix: Either build equivalent cohort × relative-time columns for pyfixest, or label the two Sun-Abraham pyfixest cells as timing-only proxy / exclude them from the headline table.

Maintainability

No findings. The added benchmark files are separated from runtime library code and share DGP construction through fe_absorption_datagen.py.

Tech Debt

No blocking findings. Existing FE-absorption optimization work is already documented as performance work; this PR adds measurement surface rather than new estimator debt.

Security

No findings. I did not find secrets, credentials, or unsafe subprocess handling in the changed benchmark files.

Documentation/Tests

Finding 3
Severity: P3
Impact: The main FE benchmark validates --repeats >= 1, but the pyfixest lane does not. Passing --repeats 0 reaches statistics.median([]) and fails with a generic error. See benchmarks/speed_review/bench_fe_absorption.py:L248-L249 versus benchmarks/speed_review/bench_fe_absorption_pyfixest.py:L77-L129.
Concrete fix: Add the same if args.repeats < 1: ap.error("--repeats must be >= 1") guard to the pyfixest script.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: ebe0fcad1aeb907ef24e635f95c5fc32b552cf2a

---

Overall Assessment

✅ Looks good. This re-review found no unmitigated P0/P1 issues.

Executive Summary

• No diff_diff/ estimator source changes; this PR is benchmark/docs/baseline-only.
• Affected methods are benchmarked only: SunAbraham, TwoWayFixedEffects, and DifferenceInDifferences(absorb=..., survey_design=...).
• Prior P2 pyfixest table issues are resolved: noisy CVs and Sun-Abraham proxy rows are now visibly labeled in docs/performance-plan.md:L56-L64.
• Prior P3 --repeats 0 issue is resolved in benchmarks/speed_review/bench_fe_absorption_pyfixest.py:L92-L93.
• Static review only; I did not run the heavy benchmark suite.

Methodology

No blocking findings.

The benchmark invocations match the relevant registry contracts: TWFE within-transform/FWL at docs/methodology/REGISTRY.md:L333-L356, DiD absorb= MAP residualization and non-convergence warning behavior at docs/methodology/REGISTRY.md:L4301-L4324, SunAbraham cohort/event-time IW setup at docs/methodology/REGISTRY.md:L1150-L1173, and BRR replicate-refit support for DiD absorb at docs/methodology/REGISTRY.md:L4463-L4521.

P3 informational: tail_stress intentionally records the documented MAP non-convergence warning and is exempted from the identity gate in benchmarks/speed_review/bench_fe_absorption.py:L55-L60 and benchmarks/speed_review/bench_fe_absorption.py:L207-L212. Impact: transparent benchmark evidence for the known max_iter=100 contract issue. Concrete fix: none required.

Code Quality

No findings. The benchmark code is isolated under benchmarks/speed_review/ and does not introduce public API or estimator behavior changes.

Performance

No findings.

Prior finding resolved: gen_findings_tables.py now derives noisy from either the stored noisy flag or fit_cv > 0.10, and labels pyfixest Sun-Abraham timing-only rows as proxy in benchmarks/speed_review/gen_findings_tables.py:L231-L245.

Maintainability

No findings. Shared DGP construction is factored into benchmarks/speed_review/fe_absorption_datagen.py:L168-L235, which keeps the diff-diff and pyfixest lanes aligned by scenario id and checksum.

Tech Debt

No blocking findings. The benchmark documents the existing MAP iteration-cap issue rather than adding estimator debt.

Security

No findings. The subprocess benchmark calls use argument vectors, not shell interpolation, in benchmarks/speed_review/bench_fe_absorption.py:L173-L188; I did not find secrets in the changed benchmark/docs/baseline content.

Documentation/Tests

No findings.

Prior findings resolved: the generated performance table now surfaces both noisy and proxy caveats at docs/performance-plan.md:L56-L64, and the pyfixest lane validates --repeats >= 1 at benchmarks/speed_review/bench_fe_absorption_pyfixest.py:L92-L93.