Skip to content

Commit 8d357b6

Browse files
igerberclaude
andcommitted
Address CI AI review round 4: narrow Phase 1b guards, strict boundary
Reviewer correctly flagged that the 1%-of-median rule is a Phase 2 design="auto" heuristic, not Phase 1b. Backed off that over-reach. P1 #1: Removed the min(d)/median(d) < 0.01 check. The mass-point guard now applies uniformly (whenever d.min() > 0 and modal fraction at d.min() > 2%) and does not gate on boundary. This still catches the original concern (silently routing mass-point data through the nonparametric branch) without rejecting valid Design 1' samples like Beta(2,2) where d.min() is strictly positive but small. P1 #2: Tightened boundary validation. The wrapper now accepts only boundary ~ 0 (Design 1') or boundary ~ d.min() (Design 1 continuous- near-d_lower) within float tolerance. Off-support values -- including the previously-allowed "boundary < d.min()" path -- are rejected with a targeted error message. P3: Added a public-wrapper duplicate-support regression that drives a rank-deficient X'X through the full selector stack (boundary = d.min(), unique minimum, only 4 distinct d values) and asserts a specific "qrXXinv" ValueError, not LinAlgError. Test updates: - Removed test_boundary_zero_with_positive_d_min_rejected: the case it modeled is now accepted (no mass point). - Added test_boundary_zero_thin_boundary_density_accepted: Beta(2,2) Design 1' with vanishing boundary density now passes. - Added test_off_support_boundary_rejected: boundary=0.5 on U(1,2). - Added test_negative_boundary_rejected: boundary<0 rejected. - Updated test_nonzero_boundary: uses boundary=float(d.min()), not boundary=1.0 (which is off the realized support of U(1,2)). 175 tests pass (up from 172). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a29a8fa commit 8d357b6

2 files changed

Lines changed: 113 additions & 121 deletions

File tree

diff_diff/local_linear.py

Lines changed: 41 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -606,100 +606,57 @@ def mse_optimal_bandwidth(
606606
raise ValueError(f"boundary must be finite; got {boundary}")
607607

608608
# Boundary-applicability check (Phase 1b scope).
609-
# The exported wrapper is scoped to the HAD LOWER-boundary case:
610-
# - Design 1' (d_0 = 0 with support infimum at 0), or
611-
# - Design 1 continuous-near-d_lower (d_0 = d_lower = min(D_2)).
612-
# In both cases ``boundary`` must lie at or below the sample minimum:
613-
# no observation should fall to the left of the evaluation point,
614-
# because the downstream ``local_linear_fit`` uses a one-sided kernel
615-
# restricted to ``d >= d_0``. An interior or upper-boundary
616-
# evaluation would silently run the boundary selector branch with a
617-
# symmetric kernel (see port's ``kernel_W``) and produce a bandwidth
618-
# incompatible with the fitter. Reject those inputs here until an
619-
# interior-point API is documented (Phase 2+).
609+
# The exported wrapper is scoped to the two documented HAD
610+
# nonparametric estimands:
611+
# - Design 1' evaluates m(0) = lim_{d down 0} E[Delta Y | D_2 <= d]
612+
# with ``boundary = 0``.
613+
# - Design 1 continuous-near-d_lower evaluates m(d_lower) with
614+
# ``boundary = d_lower = min(D_2)``.
615+
# Any other off-support boundary (interior, upper-boundary, or an
616+
# arbitrary value between 0 and d.min()) would silently target an
617+
# undocumented limit and is rejected.
620618
d_min = float(d.min())
621-
# Allow a small numerical tolerance so boundary = min(d) passes even
622-
# under floating-point noise (e.g. ``d.min()`` equals ``boundary``
623-
# within 1e-12 but not bit-exact).
624619
_boundary_tol = 1e-12 * max(1.0, abs(d_min), abs(boundary))
625-
if boundary > d_min + _boundary_tol:
620+
_at_zero = abs(boundary) <= _boundary_tol
621+
_at_d_min = abs(boundary - d_min) <= _boundary_tol
622+
if not (_at_zero or _at_d_min):
626623
raise ValueError(
627-
f"boundary={boundary!r} exceeds the sample minimum "
628-
f"d.min()={d_min!r}. The Phase 1b wrapper is scoped to the "
629-
f"HAD lower-boundary case (d_0 <= min(d)). For interior or "
630-
f"upper-boundary evaluation, use "
631-
f"diff_diff._nprobust_port.lpbwselect_mse_dpi directly with "
632-
f"interior=True; note that the non-boundary branches are not "
633-
f"separately parity-tested against nprobust."
624+
f"boundary={boundary!r} is not at a supported HAD estimand. "
625+
f"The Phase 1b public wrapper accepts only boundary ~ 0 "
626+
f"(Design 1') or boundary ~ d.min()={d_min!r} (Design 1 "
627+
f"continuous-near-d_lower). Off-support values would "
628+
f"silently target an undocumented limit. For interior or "
629+
f"other boundary points, use "
630+
f"diff_diff._nprobust_port.lpbwselect_mse_dpi directly and "
631+
f"note that those paths are not separately parity-tested."
634632
)
635633

636-
# Design classification (REGISTRY.md Phase 2 design="auto" rule
637-
# plus paper Section 3.2.4 mass-point redirection):
638-
#
639-
# Design 1' <=> d_lower = inf supp(D_2) = 0
640-
# Design 1 masspt <=> d_lower > 0, P(D_2 = d_lower) > 0
641-
# Design 1 cont <=> d_lower > 0, D_2 continuous near d_lower
642-
#
643-
# The CCF nonparametric selector is appropriate for Design 1' and
644-
# Design 1 continuous-near-d_lower. Mass-point Design 1 requires a
645-
# 2SLS sample-average estimator (Phase 2, not Phase 1b).
634+
# Mass-point design check (paper Section 3.2.4, REGISTRY 2% rule).
635+
# When d_min > 0 and there is bunching at d_min, Design 1 requires
636+
# the 2SLS sample-average path (Phase 2), not the CCF nonparametric
637+
# selector. The check applies independently of the boundary the
638+
# user supplied: mass-point data is never appropriate for this
639+
# wrapper. The check explicitly excludes d_min ~ 0, which is the
640+
# Design 1' "untreated units present" subcase that the paper's
641+
# simulations and the Garrett et al. (2020) application accept.
646642
_MASS_POINT_THRESHOLD = 0.02 # REGISTRY rule: > 2% modal-min
647-
eps_eq = 1e-12 * max(1.0, abs(d_min))
648-
at_d_min_mask = np.abs(d - d_min) <= eps_eq
649-
modal_fraction = float(np.mean(at_d_min_mask))
650-
651-
if boundary > _boundary_tol:
652-
# User supplied boundary = d_min > 0 explicitly. This is
653-
# Design 1; distinguish mass-point vs continuous-near-d_lower.
643+
if d_min > _boundary_tol:
644+
eps_eq = 1e-12 * max(1.0, abs(d_min))
645+
at_d_min_mask = np.abs(d - d_min) <= eps_eq
646+
modal_fraction = float(np.mean(at_d_min_mask))
654647
if modal_fraction > _MASS_POINT_THRESHOLD:
655648
raise NotImplementedError(
656-
f"Detected mass-point design: the lower boundary "
657-
f"d_lower={d_min!r} has modal fraction "
658-
f"{modal_fraction:.4f} > {_MASS_POINT_THRESHOLD:.2f}. "
659-
f"Per de Chaisemartin et al. (2026) Section 3.2.4 and "
660-
f"the methodology registry, this case requires the 2SLS "
661-
f"sample-average estimator with instrument 1{{D_2 > "
662-
f"d_lower}}, not the nonparametric CCF local-polynomial "
663-
f"bandwidth selector. That estimator is queued for "
649+
f"Detected mass-point design at d.min()={d_min!r} "
650+
f"(modal fraction {modal_fraction:.4f} > "
651+
f"{_MASS_POINT_THRESHOLD:.2f}). Per de Chaisemartin et "
652+
f"al. (2026) Section 3.2.4, Design 1 mass-point cases "
653+
f"require the 2SLS sample-average estimator with "
654+
f"instrument 1{{D_2 > d_lower}}, not the CCF "
655+
f"nonparametric selector. That path is queued for "
664656
f"Phase 2 (HeterogeneousAdoptionDiD). For continuous "
665657
f"near-d_lower designs (modal fraction <= "
666-
f"{_MASS_POINT_THRESHOLD:.2f}), this wrapper is applicable."
667-
)
668-
else:
669-
# User passed boundary <= 0 (default). Intent is Design 1'.
670-
# Apply the REGISTRY's `min(d) < 0.01 * median(d)` rule: only
671-
# accept when the support infimum is effectively 0 relative to
672-
# the data scale. Otherwise force the user to disambiguate.
673-
d_median = float(np.median(d))
674-
_DESIGN_1_PRIME_RATIO = 0.01 # REGISTRY: min(d)/median(d) < 1%
675-
# Guard against pathological all-zero or all-negative median.
676-
effective_threshold = _DESIGN_1_PRIME_RATIO * max(abs(d_median), 1e-12)
677-
if d_min > effective_threshold:
678-
if modal_fraction > _MASS_POINT_THRESHOLD:
679-
# Mass-point design dressed as Design 1': same
680-
# NotImplementedError as the boundary>0 branch.
681-
raise NotImplementedError(
682-
f"Detected mass-point design at d.min()={d_min!r} "
683-
f"(modal fraction {modal_fraction:.4f} > "
684-
f"{_MASS_POINT_THRESHOLD:.2f}), but boundary=0 "
685-
f"implies Design 1' intent. Either: (a) pass "
686-
f"boundary=d.min() to make the mass-point "
687-
f"classification explicit (which will then raise "
688-
f"NotImplementedError directing to the 2SLS path "
689-
f"per de Chaisemartin et al. 2026 Section 3.2.4), "
690-
f"or (b) reconsider whether the data is truly "
691-
f"Design 1' (support at 0)."
692-
)
693-
raise ValueError(
694-
f"Ambiguous design: boundary=0 but d.min()={d_min!r} "
695-
f"exceeds the Design 1' threshold of "
696-
f"0.01 * median(d) = {effective_threshold!r}. This "
697-
f"dataset does not satisfy Design 1' (support infimum "
698-
f"at 0). Either: (a) pass boundary=d.min() for the "
699-
f"Design 1 continuous-near-d_lower path, or (b) verify "
700-
f"the data truly has support at 0 (in which case "
701-
f"d.min() would be much closer to zero relative to the "
702-
f"data scale)."
658+
f"{_MASS_POINT_THRESHOLD:.2f}), this wrapper is "
659+
f"applicable."
703660
)
704661

705662
# Defer heavy import to call time to avoid import-cycle risk.

tests/test_bandwidth_selector.py

Lines changed: 72 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -313,30 +313,40 @@ def test_boundary_equal_to_min_d_accepted(self):
313313
assert h > 0.0
314314

315315
def test_boundary_zero_design_1_prime_accepted(self):
316-
"""Design 1' data: d.min() effectively 0 relative to median,
317-
so boundary=0 passes the REGISTRY 1%-of-median rule."""
316+
"""Design 1' with support at 0: boundary=0 passes."""
318317
rng = np.random.default_rng(20260419)
319-
# d.min() ~ 5e-4 << 0.01 * median(d) ~ 5e-3
320318
d = rng.uniform(0.0, 1.0, size=3000)
321319
y = d + d**2 + rng.normal(0, 0.5, size=3000)
322320
h = mse_optimal_bandwidth(d, y, boundary=0.0)
323321
assert np.isfinite(h)
324322
assert h > 0.0
325323

326-
def test_boundary_zero_with_positive_d_min_rejected(self):
327-
"""Default boundary=0 with d.min() substantially positive (>1%
328-
of median) is not Design 1'; force the caller to pass
329-
boundary=d.min() instead of silently misclassifying."""
324+
def test_boundary_zero_thin_boundary_density_accepted(self):
325+
"""Beta(2,2) Design 1' case: boundary density vanishes at 0
326+
but the estimand is well-defined. Must not be mistakenly
327+
rejected by any design heuristic."""
328+
rng = np.random.default_rng(20260419)
329+
d = rng.beta(2.0, 2.0, size=2000) # f_D(0) = 0
330+
y = d + d**2 + rng.normal(0, 0.5, size=2000)
331+
h = mse_optimal_bandwidth(d, y, boundary=0.0)
332+
assert np.isfinite(h)
333+
assert h > 0.0
334+
335+
def test_boundary_zero_with_data_far_from_zero_fails_gracefully(self):
336+
"""boundary=0 passes the boundary-validation and mass-point
337+
checks but then hits the per-stage count guard deeper in the
338+
selector because the kernel window is empty (d ~ U(0.5, 1.0)
339+
has no data near 0). Must surface a clear ValueError, not an
340+
opaque failure."""
330341
rng = np.random.default_rng(2026)
331-
# d.min() ~ 0.5, median ~ 0.75 -> ratio 0.67 >> 0.01.
332-
d = rng.uniform(0.5, 1.0, size=1500)
342+
d = rng.uniform(0.5, 1.0, size=1500) # d.min() ~ 0.5, no mass
333343
y = d + rng.normal(0, 0.3, size=1500)
334-
with pytest.raises(ValueError, match="Ambiguous design"):
344+
with pytest.raises(ValueError, match="lprobust_bw"):
335345
mse_optimal_bandwidth(d, y, boundary=0.0)
336346

337347
def test_boundary_zero_with_d_min_mass_point_rejected(self):
338-
"""boundary=0 default path with d.min() > 0 AND mass at d.min()
339-
must also be rejected, pointing to the 2SLS redirection."""
348+
"""boundary=0 with d.min() > 0 AND mass at d.min() is a
349+
Design 1 mass-point design and must be redirected to 2SLS."""
340350
rng = np.random.default_rng(2026)
341351
n_mass = 300 # 15% at 0.1
342352
n_cont = 1700
@@ -347,6 +357,25 @@ def test_boundary_zero_with_d_min_mass_point_rejected(self):
347357
with pytest.raises(NotImplementedError, match="mass-point"):
348358
mse_optimal_bandwidth(d, y, boundary=0.0)
349359

360+
def test_off_support_boundary_rejected(self):
361+
"""boundary must equal 0 or d.min() within tolerance; any
362+
other lower off-support value must be rejected."""
363+
rng = np.random.default_rng(2026)
364+
d = rng.uniform(1.0, 2.0, size=1500) # d.min() ~ 1.0
365+
y = d + rng.normal(0, 0.3, size=1500)
366+
# boundary = 0.5 is between 0 and d.min(); neither documented
367+
# estimand.
368+
with pytest.raises(ValueError, match="not at a supported HAD estimand"):
369+
mse_optimal_bandwidth(d, y, boundary=0.5)
370+
371+
def test_negative_boundary_rejected(self):
372+
"""boundary < 0 is off-support and rejected."""
373+
rng = np.random.default_rng(2026)
374+
d = rng.uniform(0.0, 1.0, size=1500)
375+
y = d + rng.normal(0, 0.3, size=1500)
376+
with pytest.raises(ValueError, match="not at a supported HAD estimand"):
377+
mse_optimal_bandwidth(d, y, boundary=-0.1)
378+
350379
def test_mass_point_design_rejected(self):
351380
"""Design 1 mass-point case (boundary > 0, modal fraction > 2%)
352381
must be rejected with NotImplementedError pointing to 2SLS."""
@@ -394,30 +423,33 @@ def test_rank_deficient_design_raises_valueerror(self):
394423
with pytest.raises(ValueError, match="qrXXinv"):
395424
qrXXinv(X)
396425

397-
def test_full_stack_rank_deficient_raises_valueerror(self):
398-
"""End-to-end rank-deficiency integration test: a sample with
399-
enough rows but only a handful of distinct in-window d values
400-
drives rank-deficient X'X at some DPI stage. The public wrapper
401-
must surface a clear ValueError (either from qrXXinv's Cholesky
402-
guard or from the stage-count guards), not IndexError or
403-
LinAlgError.
426+
def test_wrapper_rank_deficient_raises_valueerror(self):
427+
"""Public-wrapper regression: a continuous-near-d_lower sample
428+
whose kernel window contains too few DISTINCT d values drives
429+
a rank-deficient X'X in one of the DPI stages. The wrapper
430+
must surface a clear ValueError from qrXXinv's Cholesky guard,
431+
not an opaque LinAlgError.
432+
433+
Construction: d.min() is unique (modal_fraction = 1/G < 2% so
434+
mass-point check passes), but the remaining data concentrates
435+
on a single value so the kernel window has only 2 distinct d
436+
values and the design-matrix columns become linearly
437+
dependent at higher polynomial orders.
404438
"""
405-
from diff_diff._nprobust_port import lpbwselect_mse_dpi
406-
407-
# Only 3 distinct d values; each pilot stage's design matrix
408-
# will have at most 3 independent rows, but the B2 fit needs
409-
# o_B+2 = 5 or 6 columns -> rank deficient.
410-
d = np.repeat([0.1, 0.2, 0.3], 50).astype(np.float64)
411439
rng = np.random.default_rng(2026)
412-
y = d + rng.normal(0, 0.1, size=d.size)
413-
with pytest.raises(ValueError):
414-
lpbwselect_mse_dpi(y, d, eval_point=0.0, bwcheck=None)
415-
# Same through the public wrapper (boundary=0 is fine since
416-
# d.min()=0.1 triggers either ambiguous-design or rank-deficient
417-
# rejection; we only care that it is a ValueError and not an
418-
# opaque linear-algebra crash).
419-
with pytest.raises((ValueError, NotImplementedError)):
420-
mse_optimal_bandwidth(d, y)
440+
# G = 151: d.min=0.1 unique, 50 obs each at 0.15 / 0.3 / 0.4.
441+
# Modal fraction = 1/151 < 2% passes mass-point check.
442+
# The B1 / B2 auxiliary fits at stage d1 use h_B1 = h_B2 =
443+
# range = 0.3, which retains all 4 distinct values (0.1, 0.15,
444+
# 0.3, 0.4). The B1 design matrix has o_B+1 = 5 columns but
445+
# only 4 independent rows -> rank-deficient X'X -> Cholesky
446+
# fails in qrXXinv.
447+
d = np.concatenate(
448+
[[0.1], np.full(50, 0.15), np.full(50, 0.3), np.full(50, 0.4)]
449+
)
450+
y = d + rng.normal(0, 0.01, size=d.size)
451+
with pytest.raises(ValueError, match="qrXXinv"):
452+
mse_optimal_bandwidth(d, y, boundary=float(d.min()))
421453

422454

423455
class TestKernelDispatch:
@@ -446,12 +478,15 @@ def test_kernel_epa_vs_tri_differ(self):
446478

447479
class TestBoundary:
448480
def test_nonzero_boundary(self):
449-
"""Design 1 continuous-near-d_lower case: evaluate at a non-zero
450-
boundary d_0 = d_lower."""
481+
"""Design 1 continuous-near-d_lower case: boundary = d.min()
482+
(not the theoretical infimum of the support). Under the
483+
strict boundary-applicability check, the user must pass the
484+
sample minimum, not a known theoretical lower bound like 1.0
485+
on U(1, 2) data."""
451486
rng = np.random.default_rng(2026)
452487
d = rng.uniform(1.0, 2.0, size=1500)
453488
y = 3.0 + 0.5 * (d - 1.0) ** 2 + rng.normal(0, 0.3, size=1500)
454-
h = mse_optimal_bandwidth(d, y, boundary=1.0)
489+
h = mse_optimal_bandwidth(d, y, boundary=float(d.min()))
455490
assert np.isfinite(h)
456491
assert h > 0.0
457492

0 commit comments

Comments
 (0)