Refresh METHODOLOGY_REVIEW.md to reflect current estimator catalog#448
Open
igerber wants to merge 12 commits into
Open
Refresh METHODOLOGY_REVIEW.md to reflect current estimator catalog#448igerber wants to merge 12 commits into
igerber wants to merge 12 commits into
Conversation
Tracker had fallen ~9 estimators behind the library. Audit against __init__.py __all__, docs/methodology/REGISTRY.md, docs/methodology/ papers/, and tests/test_methodology_*.py surfaced four "Not Started" entries that are stale and ~10 missing entries entirely. Changes: - Reorganized Review Status Summary into seven categories (Core, Staggered, Continuous & Universal-Treatment, Triple-Difference, Counterfactual, Diagnostics, Cross-Cutting Inference Features). - Added "What 'Complete' means" tier definition (Complete / In Progress / Not Started) so the bar is explicit. - Added In-Progress entries for ImputationDiD, TwoStageDiD, WooldridgeDiD (ETWFE), EfficientDiD, ContinuousDiD, ChaisemartinDHaultfoeuille (DCDH), HeterogeneousAdoptionDiD (HAD), TROP, StaggeredTripleDifference, ConleySpatialHAC, Survey Data Support, PlaceboTests. Each "In Progress" block lists what's already in place (REGISTRY section, paper review, methodology test file, parity fixtures, test counts) and what's needed to promote to Complete. - Updated SyntheticDiD last-review date to 2026-04-23 (PR #351 bootstrap-refit landing date) to reflect the warm-start FW corrections. - Refreshed methodology-test counts on the existing Complete entries to match current `grep` output (CallawaySantAnna 61, HonestDiD 27, TripleDifference 45, DifferenceInDifferences 51, HonestDiD unit 72). - Updated Priority Order: BaconDecomposition flagged as next substantive review (chosen during this session); In-Progress promotion ladder documented (HAD largest, DCDH closest to ready, etc.). No source code changes. No status flips on existing Complete entries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Overall Assessment ✅ Looks good — no unmitigated P0/P1 findings. This is a docs-only PR, and the estimator catalog refresh itself looks directionally correct. The issues I found are tracker-accuracy problems inside Executive Summary
Methodology
Code Quality No findings. Performance No findings. Maintainability No findings beyond the documentation drift called out below. Tech Debt No findings. The PR’s non-blocking references to deferred methodology work line up with existing tracking in Security No findings. Documentation/Tests
|
P2 (tracker contract internal consistency): the new "What 'Complete'
means" definition was stricter than several existing Complete entries
satisfy (notably SyntheticDiD lacked a Verified Components block;
HonestDiD had unchecked Verified Components items and an awkward
"(pending R comparison)" status caveat; DiD / MultiPeriodDiD / TWFE /
TripleDifference lacked explicit Deviations blocks). Resolved by:
- Relaxing the legend to acknowledge format variation across the
existing Complete entries (catalog grew incrementally; the
invariant is documented walk-through against the academic source,
not a fixed structural template).
- Aspiring to the fuller structure for new reviews going forward.
- Backfilling SyntheticDiD's Verified Components block (Frank-Wolfe
on collapsed form, two-pass sparsification, auto-zeta from data
noise level, pairs-bootstrap refit with warm-start, placebo + jackknife
variance methods) and Deviations from R block (default
variance_method, parameter renaming with deprecation aliases).
- Adding brief Deviations blocks to DiD / MultiPeriodDiD / TWFE /
TripleDifference: "(None — verified equivalent)" for DiD and TWFE;
HC1-default + e=-1 reference for MultiPeriodDiD; panel=FALSE-only
for TripleDifference.
- Dropping the "(pending R comparison)" parenthetical from HonestDiD's
status field; the R-parity-pending note remains in Outstanding
Concerns where it belongs.
P3 (count drift): refreshed counts cited by the reviewer:
- StackedDiD: "72 tests across 11 test classes" -> "10 test classes"
with an explicit category list. The 72 count predated this refresh
and was stale; class count is now structurally pinned to the actual
Test* declarations.
- Survey Data Support: "8 dedicated test files" -> "13 dedicated
test files" (matches the existing enumeration).
Opportunistically replaced brittle unit-test counts in the existing
DiD / CallawaySantAnna / SunAbraham Complete entries with structural
wording ("Existing unit-test coverage in tests/test_X.py"), per the
reviewer's "less brittle wording" suggestion. Methodology-test counts
(which correspond to the Verified Components walk-through contract)
remain pinned.
No estimator, weighting, variance, or inference code changed. No
status flips on existing Complete entries.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good — no unmitigated P0/P1 findings. This remains a docs-only PR, and the prior re-review concerns about the Executive Summary
Methodology
Code Quality No findings. Performance No findings. Maintainability No findings beyond the documentation brittleness noted below. Tech Debt No findings. The PR’s deferred-work references remain properly tracked in Security No findings. Documentation/Tests
|
P3.1: WooldridgeDiD source mismatch. The "Paper review on file" bullet pointed at docs/methodology/papers/wooldridge-2023-review.md, which covers Wooldridge (2023) "Simple approaches to nonlinear DiD" -- the companion nonlinear-extension paper, NOT the primary ETWFE source Wooldridge (2025) Empirical Economics 69(5), 2545-2587. Relabeled the bullet to "Companion-paper review on file" and explicitly noted that a dedicated review for the primary ETWFE source is not yet on file. Added a new "Outstanding for promotion" row calling for docs/methodology/papers/wooldridge-2025-review.md. Updated the Priority Order section's WooldridgeDiD line with the same precise framing. P3.2: count drift in newly added entries: - HonestDiD: "72 tests in tests/test_honest_did.py (14 classes)" -> "15 test classes" with structural enumeration (DeltaSD/DeltaRM/ DeltaSDRM bounds, FLCI, ARP, CS integration, edge cases). Brittle total-count number dropped in favor of structural description. - HAD tutorials: "(16 + 32 tests)" -> reference companion test filenames instead of the soon-to-drift counts. - survey-theory.md: dropped "(805 lines)" parenthetical; line count is brittle and adds no informational value. - Survey support estimator list: expanded from 11 estimators to the complete 15 (added SunAbraham, StackedDiD, TwoStageDiD, TripleDifference and reordered for category grouping). No status flips. No estimator, weighting, variance, or inference code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. This is still a docs-only PR, and the prior Wooldridge primary-source mismatch from the last re-review is addressed. Remaining issues are P3 tracker-accuracy items only. Executive Summary
MethodologyNo findings. The PR does not modify any estimator or inference code, and the only methodology-adjacent concern from the prior re-review was resolved by clarifying Wooldridge 2023 vs. Wooldridge 2025 review status. Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings beyond the documentation brittleness noted below. Tech DebtNo findings. The new tracked references for StaggeredTripleDifference and Conley/SyntheticDiD follow-ups point to real TODO entries and remain non-blocking. SecurityNo findings. Documentation/Tests
|
P3.1: Survey support list omitted SyntheticDiD. SDiD is exported as a top-level estimator, accepts survey_design=, and has REGISTRY-documented survey support on its placebo + jackknife paths (refit bootstrap rejects survey designs in this release, but the other variance methods work). Added SyntheticDiD to the Survey Data Support estimator list and added an explicit scope clarification: scope is *estimators*; survey-capable diagnostics (BaconDecomposition Phase 3, HonestDiD survey-df handling) are tracked in their own sections. P3.2: HAD test counts drifted further (247 -> 248 in test_had_pretests.py; the 12 and 11 counts for test_bandwidth_selector.py and test_nprobust_port.py were sub-section subset counts I mis-cited as file totals -- actual file totals are 45 and 47 respectively). De-brittled by replacing hard-coded counts with structural file descriptions: test_had.py for the HAD estimator, test_had_pretests.py for the qug/stute/yatchew_hr/workflow surface, test_had_mc.py for Monte-Carlo coverage, test_had_dual_knob_deprecation.py for deprecation paths, test_bandwidth_selector.py for the public-API wrapper, and test_nprobust_port.py for the full lprobust/lpbwselect_mse_dpi port. No status flips. No estimator, weighting, variance, or inference code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. This is still a docs-only PR. The two prior P3 issues from the last re-review are addressed, and the remaining issues are minor tracker-accuracy mismatches. Executive Summary
Methodology
Code QualityNo findings. PerformanceNo findings. MaintainabilityNo separate findings beyond the documentation drift noted above. Tech DebtNo findings. The deferred items newly cited for StaggeredTripleDifference and SyntheticDiD are actually tracked in SecurityNo findings. Documentation/Tests
|
P3.1: Conley R-parity status was understated. The tracker said parity was a self-reference baseline with unpinned R values, but the repo already ships: - benchmarks/data/r_conleyreg_conley_golden.json (committed goldens) - benchmarks/R/generate_conley_golden.R (generator) - TestConleyParityR / TestConleySparseRParityForced parity-test classes asserting atol=1e-6 on the cross-sectional path - TestConleyParitySpacetime asserting ~1e-14 on the panel path (matches conleyreg::time_dist.cpp form) Reframed the Conley entry's Outstanding for promotion: the real gap is a dedicated methodology test file and a summary R-parity table in this tracker, not the R parity itself. Added a new "Documentation in place (R parity)" sub-block. P3.2: StaggeredTripleDifference entry incorrectly said no separate unit test file exists. tests/test_staggered_triple_diff.py is a dedicated unit-test suite (~680 lines covering DR/RA/IPW paths, both control-group modes, GMM weighting, event-study aggregation, edge cases). Fixed the bullet to point at the actual file. No status flips. No estimator, weighting, variance, or inference code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good. There are no unmitigated P0/P1 findings. This remains a docs-only PR, and the prior re-review issues are largely fixed, but two non-blocking tracker-accuracy issues remain. Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
P3.1: Conley R-parity bullets misstated which class covers which surface
and overstated panel parity tolerance. The previous wording bundled
TestConleyParityR with TestConleySparseRParityForced under "cross-sectional"
and claimed panel parity at ~1e-14, but:
- TestConleyParityR is cross-sectional R parity at atol=1e-6
- TestConleyParitySpacetime is panel (space-time) R parity at atol=1e-6
- TestConleySparseRParityForced is the panel R parity with sparse path
forced, also at atol=1e-6
- The machine-precision claim belongs to a method INSIDE
TestConleyParitySpacetime (test_panel_matches_block_decomposed_reference)
that cross-checks the dense path against the hand-coded block
decomposition from conleyreg::time_dist.cpp at inner tolerance atol=1e-12
Split the bullet into three accurate entries.
P3.2: Survey strata-vs-no-strata RNG-divergence pointer was wrong. I
said the documented impossibility lives in TODO.md, but the actual
documented note is in docs/methodology/REGISTRY.md HAD Stute
survey-bootstrap section ("Distributional parity, NOT bit-exact"
note). Updated the pointer and added the explanation that
distributional parity holds at large B but exact agreement at
atol=1e-10 does not.
No status flips. No estimator, weighting, variance, or inference code
changed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
P3.1: status taxonomy inconsistency. The legend defined In Progress as
"REGISTRY entry + tests + ..." but BaconDecomposition, PreTrendsPower,
PowerAnalysis, and PlaceboTests were marked Not Started despite having
REGISTRY entries and tests on file. The reviewer's two options:
tighten the In Progress definition, or relabel the four rows. Chose
the relabel path:
- Moved BaconDecomposition / PreTrendsPower / PowerAnalysis /
PlaceboTests from Not Started -> In Progress in the Status Summary.
- Converted their per-entry bodies from the old "Current state" /
"Next step for promotion" two-section format to the standard
In Progress format ("Documentation in place" / "Outstanding for
promotion") used by the other 11 In Progress entries.
- Relaxed the legend's In Progress definition to acknowledge that the
band is wide: some In Progress entries have only REGISTRY + tests
(e.g., BaconDecomposition); others have paper review + methodology
test file + R parity (e.g., DCDH). The Outstanding for promotion
sub-section is what distinguishes them.
- Restructured the Priority Order section to a single ladder split
into "Substantive-review-blocked" (needs paper review or methodology
test file or R parity first) and "Consolidation-pass-blocked"
(mostly Verified Components walk-through). BaconDecomposition stays
#1 substantive target.
P3.2: PowerAnalysis tracker overclaimed "needs primary source
confirmation". REGISTRY's PowerAnalysis section already lists Bloom
(1995) and Burlig, Preonas & Woerman (2020) as primary sources.
Removed the source-confirmation claim from the entry and the
priority-order rationale; the actual remaining gap is no paper
review, no methodology test file, no documented reference-validation
harness against pwr/DeclareDesign.
No status flips on existing Complete entries. No estimator,
weighting, variance, or inference code changed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good No P0/P1 issues. This remains a docs-only re-review; the only unmitigated item is a P3 tracker-governance inconsistency. Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
The status taxonomy was internally inconsistent across three locations: - L27-29 overview: In Progress = REGISTRY + unit tests (wide band) - L94-97 status legend: In Progress = REGISTRY entry, tests, OR paper review (looser definition) - L1157 process rule: said to add new estimators as "Not Started" stub Aligned all three to the single contract: - Not Started = no REGISTRY entry (reserved for future use; no rows currently) - In Progress = REGISTRY entry + unit tests; promotion path to Complete (wide band) - Complete = documented walk-through in this tracker Rewrote the status legend after the Status Summary table to match the overview's "What Complete means" contract verbatim, with an explicit cross-reference link. Updated the "When adding a new estimator" process rule to direct authors to add new surfaces as In Progress (with stub Documentation in place / Outstanding for promotion sub-sections), not as Not Started. No status flips on existing entries. No estimator, weighting, variance, or inference code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good Executive Summary
Methodology No findings. The PR does not change estimator code, formulas, weighting, SE computation, inference plumbing, or assumption checks. Code Quality No findings. Performance No findings. Maintainability Severity Tech Debt No findings. The deferred Security No findings. Documentation/Tests Severity |
P3.1: StaggeredTripleDifference "shares paper review with TripleDifference" was inaccurate. The two surfaces share the primary paper (Ortiz-Villavicencio & Sant'Anna 2025), but no dedicated review file exists under docs/methodology/papers/. Reworded the priority-order line to "shares the primary paper" and added a new "Outstanding for promotion" bullet for the missing review file. P3.2: Conley entry said "three adjacent paper reviews" then enumerated four (butts-2021, butts-2023, clarke-2017, colella-et-al-2019). Fixed the count word. No status flips. No estimator, weighting, variance, or inference code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good Executive Summary
MethodologyNo findings. The PR does not change estimator code, formulas, weighting, variance/SE computation, inference plumbing, or assumption checks. Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. Tech DebtNo findings. The deferred StaggeredTripleDifference parity items referenced in the tracker are present in SecurityNo findings. Documentation/Tests
|
StaggeredTripleDifference R reference was wrong. The tracker cited triplediff::staggered_ddd() (planned/non-existent), but the actual R parity assets in benchmarks/R/benchmark_staggered_triplediff.R use the existing triplediff::ddd(panel=TRUE) entrypoint plus agg_ddd() for aggregation. Updated three locations: - Status Summary row: changed R Reference to "triplediff::ddd(panel=TRUE) + agg_ddd()" - Detailed section field table: same with attribution to the benchmark script - "Outstanding for promotion" R-parity bullet: same with attribution No status flips. No estimator, weighting, variance, or inference code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
The In Progress band example overstated HAD's review maturity. The text said "DCDH and HAD" both have "a paper review, a dedicated methodology test file, and R parity fixtures", but HAD's detail block lists a dedicated tests/test_methodology_had.py as still missing under Outstanding for promotion. Rephrased the example to make the variation explicit: - DCDH: all three (paper review + methodology file + R parity) - HAD: paper review + R parity, no dedicated methodology file - ContinuousDiD: methodology file, no paper review This communicates the same point (some In Progress entries have rich scaffolding) without implying each named example has all three. No status flips. No estimator, weighting, variance, or inference code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
DCDH paper-review claim was overstated. The cited review file docs/methodology/papers/dechaisemartin-2026-review.md covers de Chaisemartin / Ciccia / D'Haultfoeuille / Knau (2026) "Difference-in-Differences Estimators When No Unit Remains Untreated" -- the 2026 universal-rollout extension that is HAD's primary source, NOT a primary-source review for DCDH itself. DCDH's core DID_M / DID_+ / DID_- and dynamic estimators are defined in: - de Chaisemartin & D'Haultfoeuille (2020) AER 110(9), 2964-2996 - de Chaisemartin & D'Haultfoeuille (2022, revised 2024) NBER WP 29873 Neither of these has a dedicated review on disk. Same pattern as the WooldridgeDiD R2 finding: a companion-paper review existed for an adjacent paper but no primary-source review for the estimator itself. Updates (three locations): 1. In Progress band example in the legend: changed "DCDH has all three (paper review + methodology file + R parity)" to specify "companion-paper review for the 2026 universal-rollout extension" so the example honestly reflects what's on disk. 2. DCDH "Documentation in place" bullet: relabeled as "Companion-paper review on file" with an explicit note that the 2026 paper is HAD's primary source, not DCDH's, and that DCDH's 2020 AER + 2022/2024 NBER WP 29873 primary sources have no reviews on file. 3. DCDH "Outstanding for promotion" gained an explicit row for the two missing primary-source reviews. 4. Priority Order line 8 (DCDH): qualified the "paper review" shorthand to specify it's a companion-paper review and that primary-source reviews are still outstanding. No status flips. No estimator, weighting, variance, or inference code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
__init__.py__all__,docs/methodology/REGISTRY.md,docs/methodology/papers/, andtests/test_methodology_*.py. Tracker had fallen ~9 estimators behind the library.Methodology references (required if estimator / math changes)
Validation
Security / privacy
Generated with Claude Code