Skip to content

Commit 1d8e16b

Browse files
igerberclaude
andcommitted
Address PR #401 R1 review (2 P3 housekeeping items)
R1 verdict was "Looks good". Two P3 housekeeping items addressed: P3 (maintainability): `summary()` and `to_dataframe(level="by_path")` re-sorted output by `frequency_rank`, dropping the user-specified order under `paths_of_interest`. Both reporting surfaces now iterate in `path_effects` insertion order so user-specified order is preserved end-to-end. Under `by_path=k`, insertion order matches descending frequency_rank so output is identical to the prior release. Adds regressions `test_paths_of_interest_order_preserved_in_to_dataframe` and `test_paths_of_interest_order_preserved_in_summary`. P3 (docs): remaining `by_path`-only language in result docs and REGISTRY: - `chaisemartin_dhaultfoeuille_results.py` `path_sup_t_bands` empty- state contract: "no bootstrap or `by_path is None`" -> "no bootstrap, or both `by_path` and `paths_of_interest` are `None`" - `chaisemartin_dhaultfoeuille_results.py` `to_dataframe(level= "by_path")` ValueError text: pass `paths_of_interest` as an alternative to `by_path=k` - `docs/methodology/REGISTRY.md:643` joint sup-t bands Note: empty-state contract updated for both selectors Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 7df7dc2 commit 1d8e16b

3 files changed

Lines changed: 76 additions & 13 deletions

File tree

diff_diff/chaisemartin_dhaultfoeuille_results.py

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -455,8 +455,9 @@ class ChaisemartinDHaultfoeuilleResults:
455455
``path_effects[path]["horizons"][l]`` and rendered as
456456
``cband_lower`` / ``cband_upper`` columns on
457457
``to_dataframe(level="by_path")``. Empty-state contract:
458-
``None`` when not requested (no bootstrap or ``by_path is None``);
459-
``{}`` when requested but no path passed both gates (``>=2``
458+
``None`` when not requested (no bootstrap, or both ``by_path``
459+
and ``paths_of_interest`` are ``None``); ``{}`` when requested
460+
but no path passed both gates (``>=2``
460461
valid horizons with finite bootstrap SE ``> 0`` AND a strict
461462
majority — more than 50% — of finite sup-t draws). Bands
462463
cover joint inference WITHIN a
@@ -1285,10 +1286,11 @@ def _render_path_effects_section(
12851286
thin,
12861287
]
12871288
)
1288-
for path in sorted(
1289-
self.path_effects.keys(),
1290-
key=lambda p: self.path_effects[p]["frequency_rank"],
1291-
):
1289+
# Iterate in path_effects insertion order so summary preserves
1290+
# the user-specified path order under `paths_of_interest`. Under
1291+
# `by_path=k`, insertion order matches descending frequency_rank
1292+
# (the enumeration sorts by count), so the rendering is identical.
1293+
for path in self.path_effects.keys():
12921294
entry = self.path_effects[path]
12931295
rank = entry["frequency_rank"]
12941296
n_groups = entry["n_groups"]
@@ -1698,9 +1700,11 @@ def to_dataframe(self, level: str = "overall") -> pd.DataFrame:
16981700
# Mirrors the linear_trends pattern above.
16991701
if self.path_effects is None:
17001702
raise ValueError(
1701-
"Path effects not available. Pass by_path=k (positive int) "
1703+
"Path effects not available. Pass by_path=k "
1704+
"(positive int) or paths_of_interest=[(...), ...] "
17021705
"to ChaisemartinDHaultfoeuille(drop_larger_lower=False, "
1703-
"by_path=k) and L_max >= 1 to fit()."
1706+
"by_path=k) (or paths_of_interest=...) and L_max >= 1 "
1707+
"to fit()."
17041708
)
17051709
if not self.path_effects:
17061710
return pd.DataFrame(
@@ -1723,10 +1727,11 @@ def to_dataframe(self, level: str = "overall") -> pd.DataFrame:
17231727
]
17241728
)
17251729
rows = []
1726-
for path in sorted(
1727-
self.path_effects.keys(),
1728-
key=lambda p: self.path_effects[p]["frequency_rank"],
1729-
):
1730+
# Iterate in path_effects insertion order so the long-format
1731+
# table preserves the user-specified path order under
1732+
# `paths_of_interest`. Under `by_path=k`, insertion order
1733+
# matches descending frequency_rank, so output is identical.
1734+
for path in self.path_effects.keys():
17301735
entry = self.path_effects[path]
17311736
rank = entry["frequency_rank"]
17321737
n_groups = entry["n_groups"]

0 commit comments

Comments
 (0)