Skip to content

Commit 5b58d5f

Browse files
igerberclaude
andcommitted
Address local-review findings + fix test_practitioner.py mock fixture
Found via local pre-PR validation (wider polymorphic-consumer pytest sweep + adversarial probe + local AI review). 1. Fix `mock_continuous_results` fixture in tests/test_practitioner.py. The fixture was setting `r.overall_se = 0.1` on a fresh `ContinuousDiDResults.__new__()` mock. Pre-PR this silently created a junk attribute (canonical SE on ContinuousDiDResults is `overall_att_se`); post-PR `overall_se` is a read-only @Property alias added by this PR, so the assignment raised `AttributeError: can't set attribute`. Changed to assign the canonical `overall_att_se` field. `_handle_continuous` in practitioner.py never reads the SE field anyway — only `_check_nan_att(results)` reads `att`/`overall_att`. 2. Lock read-only alias semantics with a regression test (`test_aliases_are_read_only` parametrized over Pattern B / C / D) so future contributors who write similar fixtures fail loudly via this test rather than via a surprise AttributeError in another suite. 3. One-sentence REGISTRY.md note that aliases are @Property descriptors and therefore do NOT appear in `dataclasses.fields()` or `dataclasses.asdict()` output, and that assignment raises AttributeError — so serializers and field-walkers continue to see only the canonical field set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d987f9e commit 5b58d5f

3 files changed

Lines changed: 53 additions & 2 deletions

File tree

docs/methodology/REGISTRY.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
This document provides the academic foundations and key implementation requirements for each estimator in diff-diff. It serves as a reference for contributors and users who want to understand the theoretical basis of the methods.
44

5-
**Result-class field naming.** Headline scalar inference fields appear under one of four native naming patterns: flat `att` / `se` / `conf_int` / `p_value` / `t_stat` (`DiDResults`, `SyntheticDiDResults`, `TROPResults`, `HeterogeneousAdoptionDiDResults`); `overall_*` (`CallawaySantAnnaResults` and the rest of the staggered family); `overall_att_*` (`ContinuousDiDResults`, where `att` and `acrt` are parallel response curves); and `avg_*` (`MultiPeriodDiDResults`). Every scalar treatment-effect result class covered by this naming contract additionally exposes the flat `att` / `se` / `conf_int` / `p_value` / `t_stat` names as read-only `@property` aliases for adapter / external-consumer compatibility (see PR for v3.3.3, motivated by `balance.interop.diff_diff`); `ContinuousDiDResults` further exposes `overall_*` aliases pointing at the ATT side. The native field is canonical for documentation, semantics, and computation — aliases are pure read-throughs and inherit the `safe_inference()` joint-NaN consistency contract automatically.
5+
**Result-class field naming.** Headline scalar inference fields appear under one of four native naming patterns: flat `att` / `se` / `conf_int` / `p_value` / `t_stat` (`DiDResults`, `SyntheticDiDResults`, `TROPResults`, `HeterogeneousAdoptionDiDResults`); `overall_*` (`CallawaySantAnnaResults` and the rest of the staggered family); `overall_att_*` (`ContinuousDiDResults`, where `att` and `acrt` are parallel response curves); and `avg_*` (`MultiPeriodDiDResults`). Every scalar treatment-effect result class covered by this naming contract additionally exposes the flat `att` / `se` / `conf_int` / `p_value` / `t_stat` names as read-only `@property` aliases for adapter / external-consumer compatibility (see PR for v3.3.3, motivated by `balance.interop.diff_diff`); `ContinuousDiDResults` further exposes `overall_*` aliases pointing at the ATT side. The native field is canonical for documentation, semantics, and computation — aliases are pure read-throughs and inherit the `safe_inference()` joint-NaN consistency contract automatically. Because aliases are `@property` descriptors (not dataclass fields), they do NOT appear in `dataclasses.fields()` or `dataclasses.asdict()` output, and assignment to an alias raises `AttributeError`; serializers and field-walkers continue to see only the canonical field set.
66

77
## Table of Contents
88

tests/test_practitioner.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,10 @@ def mock_efficient_results():
126126
def mock_continuous_results():
127127
r = ContinuousDiDResults.__new__(ContinuousDiDResults)
128128
r.overall_att = 0.4
129-
r.overall_se = 0.1
129+
# Canonical SE field on ContinuousDiDResults is overall_att_se (the ATT side).
130+
# `overall_se` is a read-only property alias since the v3.3.3 inference-field
131+
# alias surface; assigning to it raises AttributeError.
132+
r.overall_att_se = 0.1
130133
return r
131134

132135

tests/test_result_aliases.py

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,54 @@ def test_multi_period_did_aliases():
258258
assert res.t_stat == _T
259259

260260

261+
# ============================================================================
262+
# Read-only semantics
263+
# ============================================================================
264+
265+
266+
@pytest.mark.parametrize(
267+
("cls", "ovr"),
268+
[
269+
(
270+
CallawaySantAnnaResults,
271+
{
272+
"overall_att": _ATT,
273+
"overall_se": _SE,
274+
"overall_t_stat": _T,
275+
"overall_p_value": _P,
276+
"overall_conf_int": _CI,
277+
},
278+
),
279+
(ContinuousDiDResults, _continuous_did_overrides()),
280+
(
281+
MultiPeriodDiDResults,
282+
{
283+
"avg_att": _ATT,
284+
"avg_se": _SE,
285+
"avg_t_stat": _T,
286+
"avg_p_value": _P,
287+
"avg_conf_int": _CI,
288+
},
289+
),
290+
],
291+
ids=lambda v: v.__name__ if hasattr(v, "__name__") else "ovr",
292+
)
293+
def test_aliases_are_read_only(cls, ovr):
294+
"""Assigning to an alias must raise AttributeError (no setter installed).
295+
296+
Regression: a downstream test in tests/test_practitioner.py used
297+
`r.overall_se = X` on a `ContinuousDiDResults.__new__()` mock — pre-alias
298+
that silently created a junk attribute; post-alias the property correctly
299+
rejects the assignment. Locking read-only here means future contributors
300+
who write similar fixtures fail loudly via this test rather than via a
301+
surprise `AttributeError: can't set attribute` deep in another suite.
302+
"""
303+
res = cls(**_required_init_kwargs(cls, ovr))
304+
for name in ("att", "se", "conf_int", "p_value", "t_stat"):
305+
with pytest.raises(AttributeError):
306+
setattr(res, name, object())
307+
308+
261309
# ============================================================================
262310
# Cross-cutting regression — balance.interop.diff_diff adapter pattern
263311
# ============================================================================

0 commit comments

Comments
 (0)