dCDH by_path R-parity fixtures + TestDCDHDynRParityByPath

[igerber]

Summary

• Wave 1 of the by_path follow-up sequence — locks in the per-path SE convention against R DIDmultiplegtDYN 2.3.3 before downstream waves (inference completion, design variants, survey) build on it.
• Adds two new scenarios to benchmarks/data/dcdh_dynr_golden_values.json: mixed_single_switch_by_path (2 paths, by_path=2) and multi_path_reversible_by_path (4 paths, by_path=3 via a new deterministic multi-path DGP pattern in the R generator).
• New TestDCDHDynRParityByPath class in tests/test_chaisemartin_dhaultfoeuille_parity.py — 2 parity tests matching paths by tuple label and cross-checking per-path switcher counts before SE comparison.

Observed parity

• Per-path point estimates: match R exactly (1e-4 rtol on both scenarios)
• Per-path switcher counts: match R exactly on every (path, horizon) pair
• Per-path SE within Phase 2 multi-horizon envelope: scenario 13 max rtol 10.2%, scenario 14 max rtol 4.2% (class SE_RTOL = 0.12 passes both with margin)

Documented deviation

REGISTRY.md Note (Phase 3 by_path...) block now includes:

• R-parity confirmation with the observed rtol bounds
• **Deviation from R (cross-path cohort-sharing SE):** — our full-panel cohort-centered plug-in (joiners/leavers precedent) vs R's per-path re-run diverges materially when a (D_{g,1}, F_g, S_g) cohort spans multiple observed paths; the two coincide when every cohort is single-path. Practitioner guidance: interpret per-path SE as a within-full-panel marginal variance, not a per-path conditional variance.

Scenario 14's DGP is constructed to keep cohorts single-path by making path assignment a deterministic function of F_g, demonstrating the regime where Python and R agree up to envelope.

R script additions

• New extract_dcdh_by_path helper reads res$by_levels + res$by_level_1..by_level_k, capturing per-path effects / SE / CI / switcher counts per horizon.
• New "multi_path_reversible" pattern in gen_reversible with cohort-clean path assignment.
• stopifnot(packageVersion("DIDmultiplegtDYN") >= "2.3.3") at script top — by_path output slots are not version-stable per R package docs.

Wave queue updated

~/.claude/projects/.../memory/project_dcdh_by_path_next_prs.md marks Wave 1 complete with the SE-gap observation. Next up: Wave 2 (inference completion — n_bootstrap > 0, per-path placebos, per-path sup_t_bands).

Test plan

• New parity tests pass: pytest tests/test_chaisemartin_dhaultfoeuille_parity.py::TestDCDHDynRParityByPath -v
• Full dCDH parity regression: pytest tests/test_chaisemartin_dhaultfoeuille_parity.py -v (17 passed locally)
• Full dCDH behavioral + methodology suites: pytest tests/test_chaisemartin_dhaultfoeuille.py tests/test_methodology_chaisemartin_dhaultfoeuille.py (189 + 10 passed locally)
• to_dataframe(level="by_path") round-trip on R-derived data returns 6 rows × 11 columns as expected
• Fixture regenerable: Rscript benchmarks/R/generate_dcdh_dynr_test_values.R (requires R + DIDmultiplegtDYN 2.3.3 + jsonlite)

🤖 Generated with Claude Code

[github-actions]

Overall Assessment

⛔ Blocker

Executive Summary

• Affected methods: ChaisemartinDHaultfoeuille.by_path and TROP(method="local").
• The dCDH by_path additions look methodologically acceptable; the cross-path SE difference from R is explicitly documented in docs/methodology/REGISTRY.md:L641-L645, so I treat that as informational only.
• The blocker is the unrelated TROP local-method change set: it reintroduces previously fixed weighting/variance bugs in both Python and Rust.
• Those TROP changes silently affect local-TROP tuning, point estimates, and bootstrap SE. A TODO.md entry and an xfail do not mitigate that class of defect.
• The PR also removes/downgrades the regression tests that were guarding the exact TROP contracts it breaks.
• Review is diff-based; I did not run the suite in this environment.

Methodology

• P0 — rust/src/trop.rs:L550-L609 normalizes time_weights and unit_weights, but TROP Eq. 3 and the registry require raw exp(-λ × dist) weights (docs/methodology/REGISTRY.md:L1990-L1999, L2064-L2067). Impact: Rust LOOCV and Rust bootstrap consume the wrong objective scale (rust/src/trop.rs:L435-L499, L1087-L1097), which can change selected λ_nn and downstream ATT/SE. Because TROP defaults to method="local" and finite lambda_nn values are in the default grid (diff_diff/trop.py:L64-L69, L145-L147), this is a default-path silent correctness bug. Concrete fix: remove both normalization steps and restore the raw exponential outer-product weights.
• P0 — diff_diff/trop_local.py:L418-L460 reintroduces two methodology errors. First, it only assigns donor weights when D[t,j] == 0, even though the paper’s distance formula already handles treated-cell exclusion via 1{u≠t}(1-W_iu)(1-W_ju) (diff_diff/trop_local.py:L327-L357; docs/methodology/REGISTRY.md:L1990-L1999). Second, the new _precomputed branch reads original-panel Y/D instead of the function arguments; in the bootstrap flow _fit_with_fixed_lambda rebuilds resampled matrices from boot_data (diff_diff/trop_local.py:L1230-L1316) while _precomputed was populated once from the original fit (diff_diff/trop.py:L569-L570). Impact: main-fit local TROP drops same-cohort donors contrary to Eq. 3, and Python bootstrap SE is computed from stale/misaligned distances instead of the resample. Concrete fix: compute ω_j for all j != i, let the existing (1-W) mask handle treated cells, and either clear/rebuild _precomputed per bootstrap replicate or stop consulting it inside _compute_observation_weights.
• P3 — ChaisemartinDHaultfoeuille.by_path’s R deviation is documented in docs/methodology/REGISTRY.md:L641-L645, so I do not treat the dCDH portion as a defect. Impact: none. Concrete fix: none.

Code Quality

• No separate issues beyond the methodology regressions above.

Performance

• No material performance findings beyond the fact that the new TROP shortcuts trade correctness for speed, which is already covered as P0.

Maintainability

• P3 — This PR mixes unrelated TROP estimator regressions into a dCDH parity-fixture change set (diff_diff/trop_local.py, rust/src/trop.rs, tests/test_rust_backend.py, TODO.md). Impact: review, bisecting, and rollback are harder than necessary. Concrete fix: split the TROP work into a separate PR, or revert those hunks and land only the dCDH by_path changes.

Tech Debt

• P3 — The new TODO.md entry records the local-TROP divergence, but per project policy it does not mitigate the P0s because they change estimates/SE silently. Impact: none by itself; it is just not sufficient to merge the regression. Concrete fix: resolve or remove the TROP changes before merge.

Security

• No findings.

Documentation/Tests

• P2 — tests/test_rust_backend.py:L2423-L2479 converts local bootstrap parity into xfail, and the PR removes TestTROPRustEdgeCaseParity.test_local_method_main_fit_parity and test_local_method_same_cohort_donor_parity from the same file. Impact: the exact regressions above lose CI coverage, and the remaining xfail only exercises lambda_nn=np.inf, so the new finite-λ_nn normalization bug is no longer tested at all. Concrete fix: restore those two regression tests (or equivalent coverage) and keep a non-xfail local bootstrap parity test that includes at least one finite lambda_nn case such as 0.1.

Path to Approval

1. Revert or fix the TROP local-method hunks so both Python and Rust use unnormalized raw exponential weights and include all j != i donors, with treated-cell exclusion handled only by the existing (1-W) mask.
2. Remove the _precomputed cache fallthrough from _compute_observation_weights, or rebuild _precomputed from each bootstrap sample before calling _fit_with_fixed_lambda.
3. Restore the local-TROP regression guards: test_local_method_main_fit_parity, test_local_method_same_cohort_donor_parity, and a non-xfail bootstrap parity test covering both lambda_nn=np.inf and a finite lambda_nn.
4. If the goal of this PR is only the dCDH by_path parity work, drop the unrelated TROP/TODO/CHANGELOG hunks and land the dCDH subset alone.

[igerber]

Rebased onto current origin/main (PR #358 TROP local-method fixes landed after this branch was created, producing phantom "reverted TROP work" findings). Clean diff now contains only the 5 files actually changed by this PR:

 CHANGELOG.md                                     |   1 +
 benchmarks/R/generate_dcdh_dynr_test_values.R    | 153 +++++++++++++++++-
 benchmarks/data/dcdh_dynr_golden_values.json     | 193 +++++++++++++++++++++++
 docs/methodology/REGISTRY.md                     |   2 +-
 tests/test_chaisemartin_dhaultfoeuille_parity.py | 129 +++++++++++++++

No TROP / diff_diff/trop*.py / rust/src/trop.rs / tests/test_rust_backend.py / TODO.md hunks in this PR.

/ai-review

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: b40ca7bb5e34d4faa2500141507650196ee94b4c

---

Overall Assessment

✅ Looks good

Executive Summary

• Re-review scope is now limited to dCDH by_path docs, R fixture generation, golden data, and parity tests; the earlier TROP blocker is no longer present in this diff.
• Affected method in scope: ChaisemartinDHaultfoeuille.by_path. No estimator/runtime code changed in this PR.
• The only methodology deviation I found is the cross-path cohort-sharing SE difference from R, and it is explicitly documented in docs/methodology/REGISTRY.md:L641-L641, so it is informational rather than a defect.
• I did not find any unmitigated P0/P1 issues in the current diff.
• Two P2 test-harness issues remain: the new parity tests do not enforce the exact point-parity claim, and they can silently skip an SE regression if one side becomes 0/NaN.
• I could not run the suite here because pytest is not installed in this environment.

Methodology

• P3 informational — Affected method: ChaisemartinDHaultfoeuille.by_path. The cross-path cohort-sharing SE difference versus R is now explicitly documented as a deviation in docs/methodology/REGISTRY.md:L641-L641, which satisfies the review rubric. Impact: none for merge assessment. Concrete fix: none.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• P3 — The fixture generator says the by_path slots are “not version-stable” but only guards on >= 2.3.3 in benchmarks/R/generate_dcdh_dynr_test_values.R:L32-L35. Impact: regenerating the goldens on a later DIDmultiplegtDYN release can silently change the fixture schema/semantics while still passing the guard, weakening reproducibility of the parity artifact. Concrete fix: require exactly 2.3.3 (or an explicit allowlist) for regeneration and fail with a clear message otherwise.

Tech Debt

• No findings.

Security

• No findings.

Documentation/Tests

• P2 — The merged docs say the new by_path fixtures match R point estimates exactly (CHANGELOG.md:L14-L14, docs/methodology/REGISTRY.md:L641-L641), but the new parity class uses MIXED_POINT_RTOL = 0.025 for both scenarios in tests/test_chaisemartin_dhaultfoeuille_parity.py:L495-L497 and tests/test_chaisemartin_dhaultfoeuille_parity.py:L576-L594. Impact: a materially worse point-estimate regression could still pass while the documentation continues to claim exact parity. Concrete fix: tighten these two tests to POINT_RTOL = 1e-4 (or scenario-specific exact tolerances supported by the committed goldens), or relax the docs to describe the match as approximate rather than exact.
• P2 — _compare_by_path() only checks SE parity when both sides happen to be > 0 in tests/test_chaisemartin_dhaultfoeuille_parity.py:L565-L569. Impact: if Python regresses to 0/NaN SE while the R fixture remains finite, the parity test will silently skip that cell instead of failing, which weakens the main regression guard this PR is adding. Concrete fix: first assert matching finite/missing SE state for every committed (path, horizon) cell, then do the numeric pytest.approx(...) comparison.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 23259ed3657827f01f7ff9fde70559814c92a526

---

Overall Assessment

✅ Looks good

No unmitigated P0/P1 findings. Re-review scope here is the by_path parity artifact work (CHANGELOG, R fixture generation, golden data, registry note, and parity tests); no estimator/runtime code changed in this PR.

Executive Summary

• Affected method in scope: ChaisemartinDHaultfoeuille.by_path; the implementation contract in diff_diff/chaisemartin_dhaultfoeuille.py:L398-L429 and diff_diff/chaisemartin_dhaultfoeuille.py:L4959-L5139 matches the updated registry note.
• The only methodology deviation I found is the cross-path cohort-sharing SE difference versus R, and it is now explicitly documented as a Deviation from R in docs/methodology/REGISTRY.md:L641-L641, so it is mitigated under the review rubric.
• The two issues from the prior AI review are addressed: point parity is now tightened to 1e-9, and the SE check now fails on finite/missing-state mismatch instead of silently skipping cells (tests/test_chaisemartin_dhaultfoeuille_parity.py:L495-L592).
• One new P2 test-harness gap remains: the new top-k scenario still does not assert frequency_rank, even though scenario 14 was designed with unique path counts specifically to make ranking deterministic.
• One P3 maintainability issue remains in the R generator: scenario 14 comments/params still imply a stochastic p_switch-driven DGP, but the new branch is deterministic and does not read p_switch.
• I could not run the suite here because this environment lacks pytest, and importing the estimator path also fails due missing pandas.

Methodology

• P3 informational — Affected method: ChaisemartinDHaultfoeuille.by_path. The cross-path cohort-sharing SE difference versus R is now explicitly documented in docs/methodology/REGISTRY.md:L641-L641, and that note is consistent with the in-code by_path contract in diff_diff/chaisemartin_dhaultfoeuille.py:L416-L424 and diff_diff/chaisemartin_dhaultfoeuille.py:L5092-L5130. Impact: none for merge assessment; this is a documented deviation, not an undocumented methodology defect. Concrete fix: none.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• P3 — Scenario 14’s generator comments and recorded params still describe a stochastic, p_switch-driven design, but the new multi_path_reversible branch is fully deterministic and never reads p_switch (benchmarks/R/generate_dcdh_dynr_test_values.R:L113-L181, benchmarks/R/generate_dcdh_dynr_test_values.R:L620-L638). Impact: fixture provenance is misleading; future regenerations may incorrectly assume p_switch changes the scenario or that random post-switch toggling is still being exercised. Concrete fix: either remove p_switch from the scenario call/recorded params and rewrite the scenario 14 comments to describe deterministic path assignment, or actually wire p_switch into the DGP and update the docs/changelog to match.

Tech Debt

• No findings.

Security

• No findings.

Documentation/Tests

• P2 — The new parity harness still does not assert the public frequency_rank contract, even though scenario 14 was explicitly constructed with unique path frequencies to make top-k ranking unambiguous (tests/test_chaisemartin_dhaultfoeuille_parity.py:L545-L592; generator rationale at benchmarks/R/generate_dcdh_dynr_test_values.R:L144-L157). Impact: a regression in path ranking/order would silently pass as long as the selected path set and per-path effects remain correct, weakening the main regression guard for the “top-k ranking case” this PR adds. Concrete fix: assert py_path["frequency_rank"] == r_path_entry["frequency_rank"] at least for multi_path_reversible_by_path, and ideally also assert that results.to_dataframe(level="by_path") is ordered by those ranks.
• Previous test-harness findings are resolved: exact-ish point-parity enforcement and SE-state mismatch enforcement are both now present in tests/test_chaisemartin_dhaultfoeuille_parity.py:L495-L592.
• Execution note: not run locally because pytest and pandas are unavailable in this environment.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: daefda78e347896ba56562f30935315bac3c5c95

---

Overall Assessment

✅ Looks good

No unmitigated P0/P1 findings. Re-review scope here is the new by_path parity artifact work; the estimator/runtime implementation for ChaisemartinDHaultfoeuille.by_path is unchanged in this PR.

Executive Summary

• Affected method in scope: ChaisemartinDHaultfoeuille.by_path. The updated registry/changelog language matches the existing in-code by_path contract in diff_diff/chaisemartin_dhaultfoeuille.py:L398-L429 and diff_diff/chaisemartin_dhaultfoeuille.py:L4959-L5139.
• The only methodology deviation in scope is the cross-path cohort-sharing SE difference versus R, and it is now explicitly documented as a Deviation from R in docs/methodology/REGISTRY.md:L641-L641, so it is mitigated under the review rubric.
• The prior AI-review issues are addressed: the parity harness now asserts frequency_rank and finite/missing SE-state consistency in tests/test_chaisemartin_dhaultfoeuille_parity.py:L549-L606.
• The R generator/comments are now internally consistent about the deterministic multi_path_reversible DGP, and fixture generation is pinned to exact DIDmultiplegtDYN 2.3.3 in benchmarks/R/generate_dcdh_dynr_test_values.R:L32-L40 and benchmarks/R/generate_dcdh_dynr_test_values.R:L113-L181, benchmarks/R/generate_dcdh_dynr_test_values.R:L620-L644.
• I could not run the suite locally because this environment is missing numpy, pandas, and pytest.

Methodology

• P3 informational — The cross-path cohort-sharing SE divergence versus R is explicitly documented in docs/methodology/REGISTRY.md:L641-L641, and that note is consistent with the existing by_path implementation contract in diff_diff/chaisemartin_dhaultfoeuille.py:L5092-L5130. Impact: none for merge assessment; this is a documented deviation, not an undocumented methodology defect. Concrete fix: none.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• P3 informational — The nearby mixed-direction SE parity limitation remains tracked in TODO.md:L61-L61 and is not worsened by this PR’s per-path fixtures/tests. Impact: none for merge assessment. Concrete fix: none in this PR.

Security

• No findings.

Documentation/Tests

• No findings. The prior test-harness gap on frequency_rank is closed in tests/test_chaisemartin_dhaultfoeuille_parity.py:L549-L560, and the earlier SE-state blind spot is closed in tests/test_chaisemartin_dhaultfoeuille_parity.py:L585-L606.