Address residual P3 docs drift from re-audit of PR #408#424
Conversation
Restored CI reviewer caught one actionable docs-drift item: stale mentions of "plug-in-only SE" and a `survey_design` mutex that #408 itself lifted. 1. CHANGELOG paths_of_interest entry listed `survey_design` in the by_path precondition gate alongside `heterogeneity` / `design2` / `honest_did`. Both `survey_design` (lifted by #408) and `heterogeneity` (composed in by #412) have been removed from that gate. Update the parenthetical to reflect the shipped gate and note the lifted mutexes explicitly to avoid misleading readers about current behavior. 2. `_compute_path_effects` docstring described SE only as "plug-in via _plugin_se(...)". Add a dedicated SE-formation section listing the non-survey plug-in path and the new survey path (path-restricted per-period IF routed through `_survey_se_from_group_if`, replicate-weight df propagation via `replicate_n_valid_list`, and the shared post-call `_refresh_path_inference` reconciling stored inference fields against the final `df_survey`). 3. `_compute_path_placebos` docstring mirrored the same plug-in-only description. Add a parallel SE-formation section pointing back at `_compute_path_effects` for the shared survey contract. No runtime behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Overall Assessment ✅ Looks good Executive Summary
Methodology No findings. The touched docs now accurately describe the existing non-survey plug-in path, survey cell-period allocator, replicate-weight Code Quality No findings. Docs-only patch. Performance No findings. Docs-only patch. Maintainability No findings. Tech Debt No findings. No new untracked deferred work is introduced, and nothing here needs a Security No findings. Documentation/Tests
|
The CHANGELOG paths_of_interest entry referenced `chaisemartin_dhaultfoeuille.py:1118` for the by_path precondition gate, but later edits (lifting the survey_design mutex in #408 and composing in heterogeneity in #412) have drifted the actual gate location to ~L1216. Drop the numeric anchor entirely and describe the gate textually so future edits do not silently invalidate the line reference. 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. I cross-checked the touched docs against the live survey path and registry contract. The new Code Quality No findings. Docs-only patch. Performance No findings. No runtime path changed. Maintainability No findings. The previous stale hard-coded gate reference in the Tech Debt No findings. The PR does not introduce new deferred work that needs Security No findings. Documentation/Tests No findings. The patch removes the specific docs drift called out in the prior review, and the lack of test changes is reasonable for a documentation-only PR. |
…ps + stale by_path gate list in CHANGELOG Holistic re-audit of merged igerber#408 (compose `by_path` × `survey_design` Wave 4) + igerber#424 (post-merge docs-drift cleanup). Per-PR CI on igerber#424 couldn't see the combined post-PR holistic state. Local agentic codex review surfaced 2 sibling-surface test gaps + 1 real `[Unreleased]` CHANGELOG drift. **Sibling-surface coverage**: igerber#408's Wave-4 PR shipped replicate-weight regressions for `by_path` (`test_per_path_replicate_se_finite`, `test_per_path_inference_refreshes_to_lower_final_df`) but the parallel `paths_of_interest` selector only had analytical / gate / unobserved-path tests under survey. Both selectors share the same `_compute_path_effects` and `_compute_path_placebos` IF code path, so the test gap was a selector-symmetry oversight, not a methodology gap. Added: - `test_paths_of_interest_replicate_weight_per_path_se_finite` — pins finite per-horizon SE under Rao-Wu (JK1) AND the `_refresh_path_inference` contract (every per-path entry's `t_stat` matches `safe_inference` at the FINAL `df_survey`, not the per-path snapshot from before replicate-weight fits appended to the shared `_replicate_n_valid_list`). - `test_paths_of_interest_survey_design_placebo_replicate_weight` — same invariants on the `_compute_path_placebos` branch. **CHANGELOG drift**: the original `[Unreleased]` `by_path` entry (added when by_path first shipped) said `trends_linear`, `trends_nonparam`, `heterogeneity`, `design2`, `honest_did`, and `survey_design` all raise `NotImplementedError`. Each subsequent gate-lift PR shipped its own `[Unreleased]` entry, but none of them went back to update this original entry's stale gated-features list. Users reading the changelog in order get contradictory upgrade guidance. Rewrote the gates list to reflect actual current state: only `design2` + `honest_did` remain gated. Pre-existing single-CHANGELOG-cycle hygiene gap, surfaced by the igerber#408 holistic audit but applies independently of any specific subsequent PR. Holistic pilot finding NOT addressed: phantom `heterogeneity was composed in` claim in igerber#424's CHANGELOG. That's correct in real main (igerber#412 lifted the heterogeneity gate), but appears as a code/doc mismatch in the pilot because pilot construction (per `feedback_holistic_pilot_true_merge_cherry_pick_pitfall`) is `igerber#408 + igerber#424 deltas only` and doesn't include intermediate sibling PRs like igerber#412. This is a structural limitation of the strict-delta pilot pattern — no fix-PR action needed in main.
1 participant
Summary
Audit follow-up to PR #408. The restored CI reviewer caught one actionable docs-drift item: stale mentions of "plug-in-only SE" and a `survey_design` mutex that #408 itself lifted.
CHANGELOG: The `paths_of_interest` entry's mutex list still named `survey_design` and `heterogeneity` in the by_path precondition gate. Both were lifted later in the same Unreleased cycle (Compose by_path / paths_of_interest with survey_design (Wave 4 #10) #408 lifted `survey_design`, dCDH by_path Wave 5 #11: + heterogeneity (predict_het per-by_level) #412 composed in `heterogeneity`). Update to reflect the shipped gate (`drop_larger_lower / L_max / design2 / honest_did` only) and call out the lifted mutexes explicitly.
`_compute_path_effects` docstring in `chaisemartin_dhaultfoeuille.py`: described SE only as "plug-in via `_plugin_se(...)`". Add a dedicated SE-formation section listing both the non-survey plug-in path and the new survey path (path-restricted per-period IF routed through `_survey_se_from_group_if`, replicate-weight `df_survey` propagation via `replicate_n_valid_list`, and the shared post-call `_refresh_path_inference` reconciling stored inference fields against the final `df_survey`).
`_compute_path_placebos` docstring: mirrored the same plug-in-only description. Add a parallel SE-formation section pointing back at `_compute_path_effects` for the shared survey contract.
No runtime behavior change - documentation accuracy only.
Test plan
🤖 Generated with Claude Code