Skip to content

Commit daefda7

Browse files
igerberclaude
andcommitted
Address PR #360 AI review round 3: frequency_rank assert + scenario 14 doc cleanup
Two findings (1 P2 + 1 P3): - P2: `_compare_by_path` now asserts `py_path["frequency_rank"] == r_path_entry["frequency_rank"]` for every committed path. Both scenarios are constructed with unique path frequencies (scenario 13 via the mixed_single_switch pattern, scenario 14 via deterministic counts 40/25/10/5), so rank ordering is unambiguous and any regression in top-k tiebreak handling now fails explicitly instead of passing silently as long as the selected path set and per-path effects remain correct. - P3: scenario 14 generator docstring and recorded params still described the old stochastic `p_switch`-driven DGP (the pre-PR variant that blew out SE parity via cross-path cohort mixing). The `multi_path_reversible` pattern is now DETERMINISTIC: path assignment is a fixed function of F_g with counts 20/20/15/10/10/5 across the 6 F_g values. `p_switch = 0.35` dropped from both the scenario call and the `params` block in the fixture; comment block rewritten to describe the deterministic design and cite the REGISTRY note for the rationale behind the design choice. Fixture regenerated; scenario 14 params no longer carry the stale `p_switch` entry. Point and SE parity numbers unchanged (deterministic DGP produces the same treatment matrix as before). Tests pass (2/2); ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c9dfc82 commit daefda7

3 files changed

Lines changed: 27 additions & 8 deletions

File tree

benchmarks/R/generate_dcdh_dynr_test_values.R

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -618,14 +618,20 @@ scenarios$mixed_single_switch_by_path <- list(
618618
)
619619

620620
# Scenario 14: multi_path_reversible + by_path=3 (top-k ranking case).
621-
# The multi_path_reversible DGP produces 3+ distinct observed paths via
622-
# random post-switch toggling. by_path=3 exercises the top-k selection
623-
# when observed paths exceed k. n_periods=10 gives every switch_time a
624-
# complete length-(L_max+1) window.
621+
# The `multi_path_reversible` pattern is a DETERMINISTIC multi-path DGP:
622+
# path assignment is a fixed function of F_g (so every (D_{g,1}, F_g,
623+
# S_g) cohort contains switchers from a single path), path proportions
624+
# are fixed at 20/20/15/10/10/5 across the 6 F_g values, and
625+
# post-window treatment is stable at path[L_max+1]. by_path=3 exercises
626+
# top-k selection when observed paths exceed k (4 observed paths, top-3
627+
# selected). n_periods=10 gives every switch_time a complete length-
628+
# (L_max+1) window. The old `p_switch`-driven random-toggle variant
629+
# (pre-PR) blew out SE parity with R via cross-path cohort mixing;
630+
# see the REGISTRY.md `Note (Phase 3 by_path ...)` Deviation bullet.
625631
cat(" Scenario 14: multi_path_reversible_by_path\n")
626632
d14 <- gen_reversible(n_groups = N_GOLDEN, n_periods = 10,
627633
pattern = "multi_path_reversible", seed = 114,
628-
p_switch = 0.35, L_max = 3)
634+
L_max = 3)
629635
res14 <- did_multiplegt_dyn(
630636
df = d14, outcome = "outcome", group = "group", time = "period",
631637
treatment = "treatment", effects = 3, by_path = 3, ci_level = 95
@@ -634,7 +640,7 @@ scenarios$multi_path_reversible_by_path <- list(
634640
data = export_data(d14),
635641
params = list(pattern = "multi_path_reversible", n_groups = N_GOLDEN,
636642
n_periods = 10, seed = 114, effects = 3, by_path = 3,
637-
ci_level = 95, p_switch = 0.35),
643+
ci_level = 95),
638644
results = extract_dcdh_by_path(res14, n_effects = 3)
639645
)
640646

benchmarks/data/dcdh_dynr_golden_values.json

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -660,8 +660,7 @@
660660
"seed": 114,
661661
"effects": 3,
662662
"by_path": 3,
663-
"ci_level": 95,
664-
"p_switch": 0.35
663+
"ci_level": 95
665664
},
666665
"results": {
667666
"by_path": [

tests/test_chaisemartin_dhaultfoeuille_parity.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,20 @@ def _compare_by_path(self, scenario, by_path, L_max, point_rtol, se_rtol):
545545
for r_path_entry in r_by_path:
546546
path_key = self._path_key_from_r_label(r_path_entry["path"])
547547
py_path = results.path_effects[path_key]
548+
549+
# Assert the public frequency_rank contract matches R. Both
550+
# committed scenarios are constructed with unique path
551+
# frequencies (scenario 13 via mixed_single_switch pattern,
552+
# scenario 14 via deterministic counts 40/25/10/5) so rank
553+
# ordering is unambiguous and must agree; a regression in
554+
# path ranking or top-k tiebreak handling should fail here
555+
# even if the selected path set and per-path effects remain
556+
# correct.
557+
assert py_path["frequency_rank"] == r_path_entry["frequency_rank"], (
558+
f"path={path_key}: frequency_rank mismatch "
559+
f"py={py_path['frequency_rank']} vs r={r_path_entry['frequency_rank']}"
560+
)
561+
548562
for h_str, r_h in r_path_entry["horizons"].items():
549563
h = int(h_str)
550564
assert (

0 commit comments

Comments
 (0)