Repository navigation
docs(v4): PR 4c-ii-b of the 4.0 cut - methodology/benchmark prose, tracking rows and the oracle fixture's mpd_twin prune - #871
Conversation
Overall assessment: ✅ Looks goodNo unmitigated P0 or P1 findings identified in the supplied diff. Executive summary
MethodologyNo undocumented methodology mismatch found. Reviewed simultaneous-adoption TWFE event studies against the Registry’s Freyaldenhoven/Wooldridge specification, and HonestDiD input requirements against the local Rambachan–Roth source review. The new average uses (1'V1/m^2), including cross-period covariance, and delegates inference to
Code QualityNo findings. The new averaging path handles empty post-period selections, missing covariance, NaN effects, and invalid variance without introducing inline t-statistic computation. PerformanceNo findings. The benchmark continues to time the estimator fit; summary extraction remains outside the timed block. MaintainabilityNo findings. The live benchmark is clearly distinguished from the frozen scripts used by pinned historical environments. Tech Debt
SecurityNo findings in the changed code or data. Documentation/TestsNo actionable findings. The changelog fragment follows the repository convention, and the fixture documentation explains capture provenance and re-baselining. Validation completed: six changed Python files parsed successfully; changelog checks passed; reconstructing the prior fixture from the supplied diff confirmed unchanged TWFE records and provenance. Numerical tests and the reported R parity run were not independently rerun. |
…acking rows and the oracle fixture's mpd_twin prune The docs/benchmarks half of the MultiPeriodDiD removal (split from PR 4c-ii-a because the single diff exceeded the AI review input budget). REGISTRY / REPORTING / survey-theory / METHODOLOGY_REVIEW and the remaining user-doc prose (benchmarks.rst, survey-roadmap, performance-plan, CONTRIBUTING, the repo debugging notes, the rust rank-deficiency comment) stop describing the removed class as live. TODO rows 33/35/38/95/99 reworded; three DEFERRED rows that governed the class retired. Benchmarks: new live arm benchmarks/python/benchmark_twfe_event_study.py (spec="within", cluster=unit; parity vs R fixest::feols(... | unit) verified locally - ATT 2e-12, SE 0.0%, period-effect correlation 1.000); benchmark_multiperiod.py and benchmarks/refresh_2026_07 stay frozen for the pinned 3.x-wheel harness (header comment says so); speed_review and benchmark_honest drop the removed names. Oracle fixture: the mpd_twin records and the lane metadata's MPD twin fields are pruned (TWFE records byte-identical); the capture script is TWFE-only and documents the per-lane capture SHAs; README section updated.
ffe8f7c to
3ddb83e
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment: ✅ Looks goodThe prior review remains supported. No unmitigated P0 or P1 findings identified in this diff. Executive summary
MethodologyCross-checked the benchmark specification and averaging formula against the Registry’s Freyaldenhoven/Wooldridge references, and the survey benchmark’s HonestDiD inputs against the local Rambachan–Roth source review. No undocumented mismatch found. The new average correctly includes cross-period covariance through (1'V1/m^2) and delegates inference to
Code QualityNo findings. The new averaging path handles empty selections, missing covariance, NaN effects, and invalid variance. Zero or non-finite SE produces undefined inference consistently. PerformanceNo findings. Summary extraction remains outside the timed estimator fit. MaintainabilityNo findings. The live and frozen benchmark paths are clearly distinguished. Static comparison confirms the frozen Python benchmark’s executable code is unchanged. Tech Debt
SecurityNo findings in the changed code or fixture data. Documentation/TestsNo actionable findings. The changelog fragment follows repository conventions, and fixture documentation explains capture provenance and re-baselining. Static validation confirmed that the fixture changes only remove MPD records and echo keys, and that the new benchmark’s standard CLI arguments/defaults match the frozen arm. Runtime numerical behavior remains independently unverified in this environment. |
Summary
mainonce it lands). Item 4c-ii was split because the single diff exceeded the AI review input budget; nothing here is needed by a CI gate or the REGISTRY deviation rule in the same diff as the source, which is why it could be deferred.docs/methodology/REGISTRY.md(the remaining historical mentions outside the archive note and the two Notes that shipped in feat(v4): PR 4c-ii-a of the 4.0 cut - remove MultiPeriodDiD, MultiPeriodDiDResults, PeriodEffect and EventStudy #870),REPORTING.md,survey-theory.md,variance-conventions.md(one prose line; the generated table is unchanged from feat(v4): PR 4c-ii-a of the 4.0 cut - remove MultiPeriodDiD, MultiPeriodDiDResults, PeriodEffect and EventStudy #870),METHODOLOGY_REVIEW.md,docs/benchmarks.rst(prose names the live TWFE event-study arm; the committed 2026-07 refresh tables stay byte-identical with a provenance sentence),docs/survey-roadmap.md,docs/performance-plan.md,CONTRIBUTING.md, the repo debugging-notes file, the Rust rank-deficiency comment. TODO rows 33/35/38/95/99 reworded (the 4c-ii-a rows 89 and the two new rows are unchanged); three DEFERRED rows that governed the removed class are retired.benchmarks/python/benchmark_twfe_event_study.py(TwoWayFixedEffects(cluster=unit).fit(..., event_study=True, spec="within"), same CLI and output keys as the frozen script;period_effectsconverted to reference-relativeevent_timeas before; the headline is the post-period average with the former container's exact arithmetic).benchmarks/run_benchmarks.pycalls it.benchmark_multiperiod.pyandbenchmarks/refresh_2026_07/stay frozen (header comment): that harness runs the script inside pinned 3.5.3/3.7.0 venvs that predateevent_study=True.speed_review/bench_brand_awareness_survey.pyrelabels tospec="pooled"(the 3.x call passedunit=to a class that read it only for Conley, so the measured design was the pooled one);benchmark_honest.pydrops the fake per-period record type; the two R scripts update comments only.tests/fixtures/v4_twfe_event_study_oracles.jsondrops itsmpd_twinrecords and thempd_ctor/mpd_fitecho keys (everytwferecord and the provenance block byte-identical; pruned through the capture script's owndump_fixtureserializer);tests/helpers/twfe_es_oracle_lanes.pyloses the MPD twin fields;tests/_capture_v4_merge_mpd_oracles.pyis TWFE-only and documents that a re-run is a re-baseline (the independent twin is gone);tests/fixtures/README.mdrecords the per-lane capture SHAs (68e383b; d45d531 for the two clusteredhc2_bmlanes).Methodology references (required if estimator / math changes)
diff_diff/change.Validation
tests/test_twfe_event_study_decoupling.py(31 passed on python and rust). Gates on this branch: naming guard (docs lane), tracking files, variance-conventions table gate, changelog fragments, docs IA, doc snippets, guides, doc-deps, the review-script section map, the ledger matrix: 1354 passed.run_benchmarks.py --estimator multiperiod --scale small, R + fixest 0.14.2): Python vs R headline ATT differ by 2e-12, SE relative difference 0.0%, per-period effects correlation 1.000 (max att diff 3e-11),compare_estimatesPASS; the p-value/CI differ as before because R usesnobs - kwhile the surface uses its shared post-row df (not gated).Security / privacy
Changelog
changelog.d/20261025-v4-pr4c-ii-b-docs-benchmarks.md(### Internal)Follow-ups