Skip to content

Commit aae3531

Browse files
igerberclaude
andcommitted
Address PR #392 R6 review (2 P3, all non-blocking)
R6 was ✅ Looks good — 2 P3 polish items. P3 #1 — version-aware repro installer: benchmarks/R/requirements.R installed whatever CRAN currently served via install.packages, while the generator and parity test hard-pin DIDHAD == 2.0.0 / YatchewTest == 1.1.1 / nprobust == 0.5.0. A fresh R environment regenerating the goldens would have the generator's stopifnot(packageVersion == "X.Y.Z") immediately abort. Fix: add `install_pinned_version()` helper using remotes::install_version with `upgrade = "never"`, run it after the bulk CRAN install for DIDHAD/YatchewTest/nprobust. Idempotent when the correct version is already installed. Bump procedure documented in lockstep with the generator + parity-test pins. P3 #2 — exact-set parity event_times: _zip_r_python() previously asserted only that R-mapped horizons were a SUBSET of Python's event_times (missing-in-python check). Tighten to FULL SET EQUALITY: also reject horizons present in Python but absent from R's requested set ("extra_in_python"). This catches future event_study horizon-selection regressions in both directions — e.g. if our effects/placebo cap drifts and Python emits an extra row R didn't request. Stats: 540 tests pass, 0 regressions. Still no estimator changes — all P3 polish on the parity / repro infrastructure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent eb27bf5 commit aae3531

2 files changed

Lines changed: 61 additions & 9 deletions

File tree

benchmarks/R/requirements.R

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,42 @@ install_if_missing <- function(pkg) {
3636
}
3737
}
3838

39+
# PR #392 R6 P3: pinned-version installer for upstream packages whose
40+
# version is part of the parity contract. The HAD R-parity test
41+
# (`tests/test_did_had_parity.py`) and the generator
42+
# (`benchmarks/R/generate_did_had_golden.R`) hard-pin DIDHAD,
43+
# YatchewTest, and nprobust to specific versions; without a
44+
# version-aware installer here, a fresh R environment would silently
45+
# install whatever CRAN currently serves and the generator's
46+
# `stopifnot(packageVersion(...) == "X.Y.Z")` would abort.
47+
install_pinned_version <- function(pkg, version) {
48+
if (requireNamespace(pkg, quietly = TRUE) &&
49+
as.character(packageVersion(pkg)) == version) {
50+
message(sprintf("%s is already at pinned version %s.", pkg, version))
51+
return(invisible(NULL))
52+
}
53+
message(sprintf("Installing %s == %s (pinned for HAD R-parity)...", pkg, version))
54+
if (!requireNamespace("remotes", quietly = TRUE)) {
55+
install.packages("remotes", repos = "https://cloud.r-project.org/", quiet = TRUE)
56+
}
57+
remotes::install_version(
58+
pkg,
59+
version = version,
60+
repos = "https://cloud.r-project.org/",
61+
quiet = TRUE,
62+
upgrade = "never"
63+
)
64+
}
65+
66+
# HAD R-parity (PR #392) version pins. Bump these in lockstep with
67+
# the generator's `stopifnot(packageVersion(...) == "X.Y.Z")` and the
68+
# parity test's `test_metadata_versions_match` when re-anchoring.
69+
pinned_versions <- list(
70+
DIDHAD = "2.0.0",
71+
YatchewTest = "1.1.1",
72+
nprobust = "0.5.0"
73+
)
74+
3975
install_github_if_missing <- function(pkg, repo) {
4076
if (!requireNamespace(pkg, quietly = TRUE)) {
4177
message(sprintf("Installing %s from GitHub...", pkg))
@@ -52,6 +88,15 @@ install_github_if_missing <- function(pkg, repo) {
5288
message("Installing CRAN packages...")
5389
lapply(required_packages, install_if_missing)
5490

91+
# Reinforce the HAD R-parity pinned versions AFTER the bulk install
92+
# above (which may have installed any-CRAN-version of e.g. nprobust
93+
# as a transitive dep). install_pinned_version is idempotent if the
94+
# correct version is already installed.
95+
message("\nEnforcing HAD R-parity version pins...")
96+
for (pkg in names(pinned_versions)) {
97+
install_pinned_version(pkg, pinned_versions[[pkg]])
98+
}
99+
55100
# Install GitHub packages
56101
message("\nInstalling GitHub packages...")
57102
for (pkg in names(github_packages)) {

tests/test_did_had_parity.py

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -191,15 +191,22 @@ def _zip_r_python(
191191
f"Python event_times = {py_event_times}. Mapping bug?"
192192
)
193193
pairs.append((i, py_idx_by_event_time[e], rowname))
194-
# Exact-shape assertion: every R-mapped event time must be present
195-
# in Python's event_times. Length-equality is too strict (Python
196-
# may emit additional horizons R didn't request, e.g. e=0 anchor),
197-
# but every R row must find a Python counterpart.
198-
missing_in_python = set(expected_event_times) - set(py_event_times)
199-
assert not missing_in_python, (
200-
f"event_times mismatch: R requested {sorted(expected_event_times)} "
201-
f"(mapped from R IDs); Python emitted {sorted(py_event_times)}; "
202-
f"missing in Python: {sorted(missing_in_python)}."
194+
# PR #392 R6 P3: exact set equality between R-mapped horizons and
195+
# Python's event_times (was: subset inclusion). Catches
196+
# horizon-selection regressions in BOTH directions:
197+
# - missing_in_python: Python silently dropped a horizon R requested
198+
# - extra_in_python: Python emitted an extra horizon R did not
199+
# request (e.g. effects/placebo cap drift in our event_study path)
200+
expected_set = set(expected_event_times)
201+
py_set = set(py_event_times)
202+
missing_in_python = expected_set - py_set
203+
extra_in_python = py_set - expected_set
204+
assert not missing_in_python and not extra_in_python, (
205+
f"event_times set-equality mismatch: "
206+
f"R-mapped {sorted(expected_set)}; Python emitted "
207+
f"{sorted(py_event_times)}; missing in Python: "
208+
f"{sorted(missing_in_python)}; extra in Python: "
209+
f"{sorted(extra_in_python)}."
203210
)
204211
return pairs
205212

0 commit comments

Comments
 (0)