Skip to content

Commit c215447

Browse files
igerberclaude
andcommitted
Fix CI: TWFE diagnostic guard for < 2 groups/periods
The test_rank_deficient_action_error_raises_on_fitted_twfe test failed on all CI runners because the Rust backend's solve_ols handles underdetermined systems without raising ValueError (unlike the Python backend). The test relied on solve_ols raising "Fewer observations than parameters" for a 1-group panel, which is backend-specific behavior. Fix: added back the n_groups < 2 or n_times < 2 guard in _build_group_time_design (removed in Round 14, needed again now). This guard fires deterministically on both backends before solve_ols is called. Updated the test to match the guard's error message ("at least 2 groups") instead of the backend-specific solve_ols message. 120 tests still pass locally. Black, ruff clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 5b46f40 commit c215447

2 files changed

Lines changed: 19 additions & 45 deletions

File tree

diff_diff/chaisemartin_dhaultfoeuille.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1984,6 +1984,11 @@ def _build_group_time_design(
19841984
n = len(cell)
19851985
n_groups = len(groups)
19861986
n_times = len(times)
1987+
if n_groups < 2 or n_times < 2:
1988+
raise ValueError(
1989+
f"TWFE diagnostic requires at least 2 groups and 2 time periods, "
1990+
f"got {n_groups} group(s) and {n_times} period(s)."
1991+
)
19871992

19881993
# Columns: [intercept, group_1, ..., group_{G-1}, time_1, ..., time_{T-1}]
19891994
n_cols = 1 + (n_groups - 1) + (n_times - 1)

tests/test_chaisemartin_dhaultfoeuille.py

Lines changed: 14 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -372,38 +372,17 @@ def test_cluster_parameter_raises_not_implemented(self, data):
372372

373373
def test_rank_deficient_action_error_raises_on_fitted_twfe(self):
374374
"""
375-
Per Round 13: rank_deficient_action="error" must be honored on
376-
the fitted TWFE diagnostic path, not swallowed by the blanket
377-
try/except. The standalone twowayfeweights() always honors it;
378-
the fitted path must too.
379-
380-
Uses a minimal panel (1 joiner group + 1 control group, 3
381-
periods, 1 obs per cell = 6 cells total) where the FE design
382-
has more columns than cells and triggers the underdetermined-
383-
system ValueError from solve_ols.
375+
The TWFE diagnostic requires at least 2 groups and 2 periods
376+
to build a meaningful FE design. A 1-group panel triggers a
377+
ValueError from _build_group_time_design's guard, and when
378+
rank_deficient_action="error" the blanket except in fit()
379+
re-raises it instead of swallowing it as a warning.
380+
381+
This also exercises the code path where rank_deficient_action
382+
="warn" downgrades the failure to a warning so the main
383+
estimation can proceed.
384384
"""
385-
# 2 groups, 3 periods: 6 cells but the FE design has
386-
# (2-1) + (3-1) + 1 = 4 columns. That's fine.
387-
# To trigger rank-deficient: use a panel so small that the
388-
# number of cells equals the number of FE dummies.
389-
# With 3 groups, 3 periods: 9 cells, (3-1) + (3-1) + 1 = 5 columns. Not rank-deficient.
390-
# With 2 groups, 2 periods: 4 cells, (2-1) + (2-1) + 1 = 3 columns. Not rank-deficient.
391-
# Trigger via an unbalanced panel: 3 groups, 3 periods, but
392-
# group 3 only has period 0 (terminal missingness), giving
393-
# 7 cells with 3+3-1 = 5 columns. Not rank-deficient.
394-
#
395-
# Simplest route: a single-group joiner panel (1 group, 2
396-
# periods = 2 cells, but group+time dummies need 3 columns).
397-
# This also needs a control group. Use 2 groups, but one
398-
# is a singleton-period (contributing 1 cell to 1 period only).
399-
# Actually, the easiest verified trigger: 1 group, 2 periods.
400-
# solve_ols raises "Fewer observations (2) than parameters (3)."
401-
# But fit() will also raise for missing-baseline or insufficient
402-
# groups BEFORE reaching the TWFE diagnostic — so the TWFE
403-
# diagnostic must run first (it does: Step 5a).
404-
#
405-
# Use the confirmed trigger: 1 group, 2 periods, which has
406-
# 2 cells < 3 columns in the FE design.
385+
# 1 group, 2 periods: triggers "at least 2 groups" guard
407386
df = pd.DataFrame(
408387
{
409388
"group": [1, 1],
@@ -414,7 +393,7 @@ def test_rank_deficient_action_error_raises_on_fitted_twfe(self):
414393
)
415394
# rank_deficient_action="error" should propagate through
416395
est = ChaisemartinDHaultfoeuille(twfe_diagnostic=True, rank_deficient_action="error")
417-
with pytest.raises(ValueError, match="Fewer observations"):
396+
with pytest.raises(ValueError, match="at least 2 groups"):
418397
est.fit(
419398
df,
420399
outcome="outcome",
@@ -423,17 +402,10 @@ def test_rank_deficient_action_error_raises_on_fitted_twfe(self):
423402
treatment="treatment",
424403
)
425404

426-
# rank_deficient_action="warn" should NOT raise on the same panel
427-
# (the diagnostic fails gracefully and main estimation continues)
405+
# rank_deficient_action="warn" should NOT raise the TWFE error
428406
est_warn = ChaisemartinDHaultfoeuille(twfe_diagnostic=True, rank_deficient_action="warn")
429407
with warnings.catch_warnings(record=True):
430408
warnings.simplefilter("always")
431-
# The estimation may still raise for other reasons (e.g.,
432-
# no switching cells after the 1-group panel has no controls).
433-
# What we're testing is that the TWFE diagnostic does NOT
434-
# raise. If the main estimation raises, that's fine — the
435-
# test goal is that rank_deficient_action="warn" doesn't
436-
# propagate the ValueError.
437409
try:
438410
est_warn.fit(
439411
df,
@@ -444,11 +416,8 @@ def test_rank_deficient_action_error_raises_on_fitted_twfe(self):
444416
)
445417
except ValueError as exc:
446418
# Acceptable if the error is from main estimation
447-
# (not from the TWFE diagnostic)
448-
assert "Fewer observations" not in str(exc), (
449-
"rank_deficient_action='warn' should not raise the "
450-
"TWFE rank-deficiency error"
451-
)
419+
# (not from the TWFE diagnostic guard)
420+
assert "at least 2 groups" not in str(exc)
452421

453422

454423
# =============================================================================

0 commit comments

Comments
 (0)