Skip to content

Commit 245f194

Browse files
igerberclaude
andcommitted
Address PR #401 R2 review (1 P3 maintainability)
R2 verdict was "Looks good". One P3 housekeeping item addressed: P3 (maintainability): selector-aware messaging on the empty-state path: 1. `_compute_path_effects` empty-selection warning now branches on `paths_of_interest is not None`. Under `paths_of_interest`, the message reads "every user-specified path was either unobserved ... per-path 'zero observed groups' UserWarnings already issued" so the user gets a clear pointer to the per-path warnings rather than a generic "no observed treatment path" message that misdescribes the cause. Under `by_path=k`, the original message is preserved. 2. `summary()` empty-state header changed from "Treatment-Path Disaggregation (by_path)" to "Treatment-Path Disaggregation"; the trailing parenthetical now says "by_path / paths_of_interest was a no-op on this panel". Adds regression `test_paths_of_interest_all_unobserved_emits_distinct_warning` asserting the paths_of_interest-specific warning text fires when every requested path is unobserved. Also fixes 7 pre-existing F541 f-string-without-placeholder warnings in the warning blocks via `ruff --fix`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1d8e16b commit 245f194

3 files changed

Lines changed: 62 additions & 12 deletions

File tree

diff_diff/chaisemartin_dhaultfoeuille.py

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5807,16 +5807,34 @@ def _compute_path_effects(
58075807
)
58085808

58095809
if not selected_paths:
5810-
warnings.warn(
5811-
f"by_path / paths_of_interest was requested but no observed treatment "
5812-
f"path has a complete window [F_g-1, F_g-1+L_max={L_max}] "
5813-
f"within the panel. results.path_effects is populated as an "
5814-
f"empty dict to signal 'requested but empty'. Extend the "
5815-
f"panel so switchers have L_max+1 consecutive observed cells "
5816-
f"starting at F_g-1, or reduce L_max.",
5817-
UserWarning,
5818-
stacklevel=2,
5819-
)
5810+
if paths_of_interest is not None:
5811+
# Every requested path was unobserved (each emitted its own
5812+
# per-path "zero observed groups" warning inside the
5813+
# enumerator). Distinguish from the by_path=k case where
5814+
# the panel itself has no complete-window path.
5815+
warnings.warn(
5816+
"paths_of_interest was requested but every "
5817+
"user-specified path was either unobserved in the "
5818+
"panel or had a window outside the L_max+1 "
5819+
"convention (per-path 'zero observed groups' "
5820+
"UserWarnings already issued). results.path_effects "
5821+
"is populated as an empty dict to signal 'requested "
5822+
"but empty'.",
5823+
UserWarning,
5824+
stacklevel=2,
5825+
)
5826+
else:
5827+
warnings.warn(
5828+
f"by_path={by_path} was requested but no observed "
5829+
f"treatment path has a complete window [F_g-1, "
5830+
f"F_g-1+L_max={L_max}] within the panel. "
5831+
f"results.path_effects is populated as an empty dict "
5832+
f"to signal 'requested but empty'. Extend the panel "
5833+
f"so switchers have L_max+1 consecutive observed "
5834+
f"cells starting at F_g-1, or reduce L_max.",
5835+
UserWarning,
5836+
stacklevel=2,
5837+
)
58205838
return {}
58215839

58225840
# Cohort ids for the variance-eligible set (same construction as the

diff_diff/chaisemartin_dhaultfoeuille_results.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1270,10 +1270,11 @@ def _render_path_effects_section(
12701270
lines.extend(
12711271
[
12721272
thin,
1273-
"Treatment-Path Disaggregation (by_path)".center(width),
1273+
"Treatment-Path Disaggregation".center(width),
12741274
thin,
12751275
" No observed paths have a complete [F_g-1, F_g-1+L_max] window.",
1276-
" (See UserWarning emitted at fit(); by_path was a no-op " "on this panel.)",
1276+
" (See UserWarning emitted at fit(); by_path / "
1277+
"paths_of_interest was a no-op on this panel.)",
12771278
thin,
12781279
"",
12791280
]

tests/test_chaisemartin_dhaultfoeuille.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8732,6 +8732,37 @@ def test_paths_of_interest_frequency_rank_is_true_frequency(self):
87328732
assert res.path_effects[(0, 1, 1, 1)]["frequency_rank"] == 1
87338733
assert res.path_effects[(0, 1, 0, 0)]["frequency_rank"] == 2
87348734

8735+
def test_paths_of_interest_all_unobserved_emits_distinct_warning(self):
8736+
"""When every path in `paths_of_interest` is unobserved,
8737+
the empty-state warning should mention `paths_of_interest`
8738+
explicitly rather than the generic `by_path={n}` text
8739+
(regression for R2 P3 maintainability finding)."""
8740+
df = _by_path_three_path_data()
8741+
est = ChaisemartinDHaultfoeuille(
8742+
drop_larger_lower=False,
8743+
paths_of_interest=[(1, 1, 1, 1), (1, 0, 1, 0)], # both unobserved
8744+
twfe_diagnostic=False,
8745+
seed=42,
8746+
)
8747+
with warnings.catch_warnings(record=True) as recorded:
8748+
warnings.simplefilter("always", UserWarning)
8749+
res = est.fit(
8750+
df, outcome="outcome", group="group", time="period",
8751+
treatment="treatment", L_max=3,
8752+
)
8753+
# The summary empty-state warning mentions paths_of_interest, not by_path
8754+
empty_state_warnings = [
8755+
str(w.message)
8756+
for w in recorded
8757+
if "paths_of_interest was requested but every" in str(w.message)
8758+
]
8759+
assert len(empty_state_warnings) >= 1, (
8760+
f"Expected paths_of_interest-specific empty-state warning, "
8761+
f"got: {[str(w.message) for w in recorded]}"
8762+
)
8763+
# And the result is empty dict, not None
8764+
assert res.path_effects == {}
8765+
87358766
def test_unobserved_path_warns_and_omits(self):
87368767
df = _by_path_three_path_data()
87378768
est = ChaisemartinDHaultfoeuille(

0 commit comments

Comments
 (0)