Skip to content

Commit 8f00e97

Browse files
igerberclaude
andcommitted
linalg: address CI codex R8 P2 (single-contrast noise-floor guard)
CI codex on PR #475 (✅ verdict) flagged a real P2: the noise-floor NaN- guard in `_cr2_bm_dof_inner_weighted` was batch-relative only — for a single-contrast call to `_compute_cr2_bm_contrast_dof`, `max|P|_overall` equals the contrast's own max|P|, so the `1e-10 × max|P|_overall` rule could never classify it as degenerate. That left direct single-contrast weighted callers (e.g., MPD avg_att) unprotected: they could still emit BLAS-implementation-dependent finite DOF on noise-floor contrasts even though the registry/changelog said the helper was guarded. Fix: union the batch-relative criterion with an absolute floor scaled to the bread matrix's magnitude: `(EPS × n × k × max(bread_inv_scale, 1))²`. This covers the worst-case dgemm accumulation roundoff floor for `H1/H2/H3 @ contrast` products. A single-contrast call now correctly fires the NaN-guard on a high-leverage FE-dummy contrast. New regression tests in `tests/test_methodology_wls_cr2.py:: TestWLSCR2SingleContrastNoiseFloor` (2 tests): single weighted FE-dummy contrast triggers NaN-guard + warning; single non-noise contrast still returns finite DOF matching clubSandwich at atol=1e-10. CI codex P3 (perf): LinearRegression.fit() pays CR2 twice on the new weighted hc2_bm path (solve_ols + compute_robust_vcov). Added as a TODO follow-up row (PR #475 follow-up, Low priority). All 198 linalg+methodology+estimators-vcov-type+TWFE+SA tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent dad4f68 commit 8f00e97

3 files changed

Lines changed: 129 additions & 19 deletions

File tree

TODO.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ Deferred items from PR reviews that were not addressed before merge.
108108
scenarios in benchmarks/data/clubsandwich_cr2_golden.json. -->
109109
| ~~Weighted one-way Bell-McCaffrey (`vcov_type="hc2_bm"` + `weights`, no cluster)~~ — LIFTED via clubSandwich WLS-CR2 port. Routes through `_compute_cr2_bm_contrast_dof` with singleton-cluster reduction (each obs is its own cluster). See REGISTRY.md Phase 1a row for the algebra. | `linalg.py` | LIFTED ||
110110
| ~~Weighted CR2 Bell-McCaffrey cluster-robust (`vcov_type="hc2_bm"` + `cluster_ids` + `weights`)~~ — LIFTED via clubSandwich WLS-CR2 port. `_compute_cr2_bm` now accepts `weights=` and threads through clubSandwich's specific WLS-CR2 algebra. | `linalg.py::_compute_cr2_bm` | LIFTED ||
111+
| `LinearRegression.fit()` pays the CR2 cost twice on the weighted `hc2_bm` path: once inside `solve_ols(..., return_vcov=True)` and again via `compute_robust_vcov(..., return_dof=True)` to populate `_bm_dof`. Correct but redundant. Fix: thread `return_dof` through `solve_ols` so the same CR2 computation produces both vcov + DOF, or cache the per-cluster `A_g` / `MUWTWUM` precomputes between calls. CI codex P3 on PR #475. | `linalg.py::LinearRegression.fit`, `linalg.py::solve_ols` | PR #475 follow-up | Low |
111112
| `TwoWayFixedEffects(vcov_type in {"hc2","hc2_bm"})` with replicate-weight survey designs raises `NotImplementedError` (`twfe.py:~233`). The replicate path re-demeans per replicate (re-demeaning depends on the per-replicate weight vector), which doesn't compose with the full-dummy HC2/HC2-BM build — a correct implementation would need per-replicate full-dummy refit. Workaround: use `vcov_type="hc1"` for replicate-weight CR1. | `twfe.py::fit` | follow-up | Low |
112113
| TWFE's HC2/HC2-BM inline full-dummy build (`twfe.py:280-315`) duplicates the dummy-construction logic in `DifferenceInDifferences(fixed_effects=...)` (`estimators.py:478-486`). Extract a shared helper (or delegate TWFE's HC2/HC2-BM path to DiD's `fixed_effects=` branch, with TWFE-specific cluster default threading) to reduce drift risk on FE naming, survey behavior, and result-surface conventions. Substantive refactor — touches both estimators. | `twfe.py::fit`, `estimators.py::DifferenceInDifferences.fit` | follow-up | Low |
113114
| Unify Rust local-method `estimate_model` solver path to `solve_wls_svd` (the same SVD helper used by the global-method since PR #348) for sub-1e-14 bootstrap SE parity. Current local-method bootstrap parity test (`tests/test_rust_backend.py::TestTROPRustEdgeCaseParity::test_bootstrap_seed_reproducibility_local`) passes at `atol=1e-5` — the residual ~1e-7 gap is roundoff between Rust's `estimate_model` matrix factorization and numpy's `lstsq`, which accumulates differently across per-replicate bootstrap fits. Main-fit ATT parity is regime-dependent (`atol=1e-14` for `lambda_nn=inf`, `atol=1e-10` for finite `lambda_nn` — see `test_local_method_main_fit_parity`); the bootstrap gap is a same-solver-path roundoff concern and not a user-visible correctness bug. | `rust/src/trop.rs::estimate_model`, `rust/src/linalg.rs::solve_wls_svd` | follow-up | Low |

diff_diff/linalg.py

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1859,27 +1859,46 @@ def _factor_psd(M):
18591859
# different specific values in this regime due to its own BLAS reduction
18601860
# order; we return NaN with a warning rather than ship arbitrarily-
18611861
# different small-sample DOF on the user-facing surface.
1862+
# The noise-floor detector has two criteria, applied union-wise:
1863+
# 1. Batch-relative: a contrast's max(|P|) < 1e-10 × the largest contrast's
1864+
# max(|P|). Useful when computing per-coefficient DOF for an entire design
1865+
# (`contrasts=np.eye(k)`) — a single noise-floor coefficient stands out.
1866+
# 2. Absolute (single-contrast safe): max(|P|) < eps_floor based on the
1867+
# bread matrix scale. Necessary because batch-relative reduces to
1868+
# max|P| < 1e-10 × max|P| (i.e. False) for a single contrast, leaving
1869+
# direct `_compute_cr2_bm_contrast_dof(...)` callers (e.g., MPD avg_att)
1870+
# unprotected. Threshold: `(EPS × n × k × bread_inv_scale)²` covers the
1871+
# worst-case dgemm accumulation roundoff floor for `H1/H2/H3 @ contrast`
1872+
# products. CI codex flagged this as P2 (R8 round); regression test in
1873+
# tests/test_methodology_wls_cr2.py::TestWLSCR2FEDoFNoiseGuard.
1874+
_EPS = np.finfo(np.float64).eps
1875+
n_obs, k_X = X.shape
1876+
bread_inv_scale = float(np.max(np.abs(bread_inv))) if bread_inv.size else 1.0
1877+
abs_noise_floor = (_EPS * n_obs * k_X * max(bread_inv_scale, 1.0)) ** 2
1878+
18621879
if m > 1:
18631880
max_P_overall = float(np.max(max_abs_P_arr))
1864-
if max_P_overall > 0:
1865-
noise_floor = 1e-10 * max_P_overall
1866-
degenerate = max_abs_P_arr < noise_floor
1867-
n_degenerate = int(np.sum(degenerate))
1868-
if n_degenerate > 0:
1869-
dof_vec[degenerate] = np.nan
1870-
warnings.warn(
1871-
f"Satterthwaite DOF for {n_degenerate} of {m} contrast(s) "
1872-
f"is at the float64 noise floor (max|P| < 1e-10 × "
1873-
f"max|P|_overall); reporting NaN. This typically affects "
1874-
f"high-leverage FE-dummy coefficients whose contrast "
1875-
f"vector projects to near-zero on the design — the "
1876-
f"resulting DOF varies across BLAS implementations and is "
1877-
f"unreliable. The coefficient SEs remain valid; only the "
1878-
f"Satterthwaite DOF (and any t-test or CI that depends on "
1879-
f"it) is suppressed.",
1880-
UserWarning,
1881-
stacklevel=3,
1882-
)
1881+
relative_floor = 1e-10 * max_P_overall if max_P_overall > 0 else 0.0
1882+
else:
1883+
relative_floor = 0.0
1884+
noise_floor = max(relative_floor, abs_noise_floor)
1885+
degenerate = max_abs_P_arr < noise_floor
1886+
n_degenerate = int(np.sum(degenerate))
1887+
if n_degenerate > 0:
1888+
dof_vec[degenerate] = np.nan
1889+
warnings.warn(
1890+
f"Satterthwaite DOF for {n_degenerate} of {m} contrast(s) "
1891+
f"is at the float64 noise floor (max|P| < noise_floor = "
1892+
f"max({relative_floor:.3e}, {abs_noise_floor:.3e})); reporting "
1893+
f"NaN. This typically affects high-leverage FE-dummy "
1894+
f"coefficients whose contrast vector projects to near-zero on "
1895+
f"the design — the resulting DOF varies across BLAS "
1896+
f"implementations and is unreliable. The coefficient SEs "
1897+
f"remain valid; only the Satterthwaite DOF (and any t-test or "
1898+
f"CI that depends on it) is suppressed.",
1899+
UserWarning,
1900+
stacklevel=3,
1901+
)
18831902

18841903
return dof_vec
18851904

tests/test_methodology_wls_cr2.py

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,96 @@ def test_did_absorbed_fe_nan_guards_treated_unit_dummies(self, goldens):
533533
), f"Expected 1 noise-floor UserWarning, got {len(noise_warnings)}"
534534

535535

536+
class TestWLSCR2SingleContrastNoiseFloor:
537+
"""Regression for CI codex R8 P2: the noise-floor NaN-guard must trigger
538+
on single-contrast calls to `_compute_cr2_bm_contrast_dof` too, not just
539+
batched per-coefficient calls. The earlier batch-relative-only criterion
540+
couldn't catch single contrasts because `max|P|_overall == contrast's own
541+
scale` for m=1. The absolute (eps × n × k × bread_scale)² floor closes
542+
the gap."""
543+
544+
def test_single_fe_dummy_contrast_triggers_nan_guard(self, goldens):
545+
"""A direct single-contrast call for a high-leverage FE-dummy unit
546+
dummy must return NaN, not a BLAS-dependent finite DOF."""
547+
import warnings
548+
549+
d = goldens["weighted_did_absorbed_fe"]
550+
n = len(d["y"])
551+
unit_int = np.asarray(d["unit"])
552+
period_int = np.asarray(d["period"])
553+
treat_post = np.asarray(d["treat_post"])
554+
np.asarray(d["y"])
555+
w = np.asarray(d["weights"])
556+
cluster = unit_int
557+
558+
cols = [np.ones(n), treat_post.astype(float)]
559+
col_names = ["(Intercept)", "treat_post"]
560+
for u in sorted(np.unique(unit_int))[1:]:
561+
cols.append((unit_int == u).astype(float))
562+
col_names.append(f"unit{u}")
563+
for p in sorted(np.unique(period_int))[1:]:
564+
cols.append((period_int == p).astype(float))
565+
col_names.append(f"period{p}")
566+
X = np.column_stack(cols)
567+
bread = X.T @ (X * w[:, None])
568+
569+
# Single contrast = unit2 dummy (the noise-floor case from the
570+
# weighted_did_absorbed_fe fixture).
571+
unit2_idx = col_names.index("unit2")
572+
c_unit2 = np.zeros(X.shape[1])
573+
c_unit2[unit2_idx] = 1.0
574+
contrasts = c_unit2[:, None] # (k, 1) — single contrast
575+
576+
with warnings.catch_warnings(record=True) as ws:
577+
warnings.simplefilter("always")
578+
dof = _compute_cr2_bm_contrast_dof(X, cluster, bread, contrasts, weights=w)
579+
assert np.isnan(dof[0]), (
580+
"Single weighted FE-dummy contrast must trigger absolute noise-"
581+
"floor guard; got finite DOF instead."
582+
)
583+
noise_warnings = [w for w in ws if "noise floor" in str(w.message)]
584+
assert (
585+
len(noise_warnings) == 1
586+
), f"Expected 1 noise-floor UserWarning, got {len(noise_warnings)}"
587+
588+
def test_single_non_noise_contrast_returns_finite_dof(self, goldens):
589+
"""Sanity check: a single contrast that's NOT at the noise floor
590+
(e.g., treat_post) must return finite DOF, not NaN."""
591+
d = goldens["weighted_did_absorbed_fe"]
592+
n = len(d["y"])
593+
unit_int = np.asarray(d["unit"])
594+
period_int = np.asarray(d["period"])
595+
treat_post = np.asarray(d["treat_post"])
596+
np.asarray(d["y"])
597+
w = np.asarray(d["weights"])
598+
cluster = unit_int
599+
600+
cols = [np.ones(n), treat_post.astype(float)]
601+
col_names = ["(Intercept)", "treat_post"]
602+
for u in sorted(np.unique(unit_int))[1:]:
603+
cols.append((unit_int == u).astype(float))
604+
col_names.append(f"unit{u}")
605+
for p in sorted(np.unique(period_int))[1:]:
606+
cols.append((period_int == p).astype(float))
607+
col_names.append(f"period{p}")
608+
X = np.column_stack(cols)
609+
bread = X.T @ (X * w[:, None])
610+
611+
# Single contrast = treat_post (NOT a noise-floor contrast).
612+
treat_post_idx = col_names.index("treat_post")
613+
c_treat = np.zeros(X.shape[1])
614+
c_treat[treat_post_idx] = 1.0
615+
contrasts = c_treat[:, None]
616+
617+
dof = _compute_cr2_bm_contrast_dof(X, cluster, bread, contrasts, weights=w)
618+
assert np.isfinite(dof[0]) and dof[0] > 0, (
619+
f"Single non-noise contrast should return finite positive DOF; " f"got {dof[0]}"
620+
)
621+
# Sanity: should match R's df_satt for treat_post (3.754) at atol=1e-10.
622+
r_dof_treat_post = float(np.asarray(d["dof_cr2"])[treat_post_idx])
623+
np.testing.assert_allclose(dof[0], r_dof_treat_post, atol=1e-10)
624+
625+
536626
class TestLinearRegressionFENanGuardEndToEnd:
537627
"""Regression for R7 P0: NaN BM DOF from the noise-floor guard must
538628
propagate to NaN inference fields in `LinearRegression.get_inference()`,

0 commit comments

Comments
 (0)