Skip to content

Commit e258ecd

Browse files
committed
Fix #406 holistic audit residuals: alias-doc completeness + test helper bugs
Holistic re-audit of merged #406 (inference-field aliases on staggered result classes) + #428 (post-merge cleanup adding TripleDifferenceResults to alias mapping examples). Per-PR CI on #428 couldn't see the combined post-PR holistic state. Local agentic codex review surfaced residuals across 4 rounds (R1-R4); a 5th round flagged Sphinx autosummary regen which is auto-handled on next docs build (not addressed here). **Test helper (R2)** — `_required_init_kwargs()` in `tests/test_result_aliases.py` had two bugs that are masked today but brittle as result dataclasses evolve: - Default-factory not honored: the `f.default is not f.default_factory and f.default is not MISSING` check returns False for factory-only fields (where default is MISSING and default_factory is a real callable), so the helper pre-filled those fields with a sentinel and the factory never ran. Replaced with explicit `_dc.MISSING` checks on both default and default_factory. - Dispatch order: `"float"` matched before `"Tuple"`, so `Tuple[float, float]` annotations were classified as scalar and got `0.0` instead of `(0.0, 0.0)`. Reordered the synthetic-value dispatch so container annotations (Tuple/List/Dict/DataFrame/ndarray) are checked before the scalar fallback. **Read-only assertion (R1)** — `test_aliases_are_read_only` checked `setattr`-fails on the flat aliases but not on the new `ContinuousDiDResults.overall_*` aliases. Extended the Continuous-specific branch. **Bundled guide documentation (R3)** — REGISTRY and CHANGELOG made the flat alias contract official, but bundled `practitioner.py`, `llms-practitioner.txt`, and `llms-full.txt` documented only the canonical `overall_*` / `overall_att_*` / `avg_*` names. Added a single "Flat-alias compatibility note" under the `## Results Objects` header in `llms-full.txt` (avoids per-class table bloat); extended the result-class snippet in `llms-practitioner.txt`; clarified the Step-4 covariate-comparison snippet in `practitioner.py`. **Registry scope correction (R4)** — the top-level REGISTRY note said "every scalar treatment-effect result class" exposes flat aliases, but flat-native classes (`DiDResults`, `SyntheticDiDResults`, `TROPResults`, `TripleDifferenceResults`, `HeterogeneousAdoptionDiDResults`) already carry these as native dataclass fields — they're unchanged by the alias contract. Narrowed the note to scope only the prefixed families. **Typo fix (R4)** — the R3 `llms-practitioner.txt` note named a nonexistent `overall_att_att` field. Replaced with the actual canonical field names (`overall_att` is the point estimate; `overall_att_se` etc. are the inference fields). 5 files, +51/-13. No behavior change; all edits are documentation alignment and test helper / test coverage hardening on the surface #406 + #428 already established.
1 parent d5e5021 commit e258ecd

5 files changed

Lines changed: 51 additions & 13 deletions

File tree

diff_diff/guides/llms-full.txt

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -967,6 +967,18 @@ results = bacon_decompose(data, outcome='y', unit='id', time='t', first_treat='f
967967

968968
## Results Objects
969969

970+
**Flat-alias compatibility note.** Every staggered result class in this
971+
section (those with canonical `overall_*` / `overall_att_*` / `avg_*`
972+
prefixed inference fields) ALSO exposes the unprefixed flat names
973+
`att` / `se` / `conf_int` / `p_value` / `t_stat` as read-only `@property`
974+
aliases over the canonical fields. The canonical prefixed fields remain
975+
the documented and computed surface; the flat aliases are pure
976+
read-throughs for compatibility with external adapters that
977+
`getattr(res, "se", None)`-style query the inference surface (e.g.
978+
`balance.interop.diff_diff.as_balance_diagnostic()`). Tables below list
979+
the canonical names; assume the flat aliases are present on every
980+
staggered class unless explicitly noted otherwise.
981+
970982
### DiDResults
971983

972984
Returned by `DifferenceInDifferences.fit()` and `TwoWayFixedEffects.fit()`.

diff_diff/guides/llms-practitioner.txt

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,16 @@ results.cohort_effects # Per-cohort effects (via to_dataframe(level='coho
7070
# Other staggered (ImputationDiD, TwoStageDiD, etc.):
7171
results.overall_att # Overall ATT
7272
results.overall_se # Standard error
73+
74+
# Flat-alias compatibility: every staggered class above (canonical
75+
# `overall_*` / `overall_att_*` / `avg_*` prefixed) also exposes the
76+
# unprefixed flat names `att` / `se` / `conf_int` / `p_value` /
77+
# `t_stat` as read-only `@property` aliases over the canonical fields.
78+
# The canonical prefixed names remain the documented and computed
79+
# surface; the flat aliases are pure read-throughs for compatibility
80+
# with adapters that `getattr(res, "se", None)`-style query inference.
81+
results.att # alias of overall_att / avg_att (or overall_att for ContinuousDiD ATT side)
82+
results.se # alias of overall_se / avg_se (or overall_att_se for ContinuousDiD ATT side)
7383
```
7484

7585
---

diff_diff/practitioner.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,8 @@ def _covariates_step() -> Dict[str, Any]:
271271
"# Re-estimate without covariates and compare:\n"
272272
"result_no_cov = estimator.fit(data, ..., covariates=None)\n"
273273
"# Compare ATT with and without covariates.\n"
274-
"# Use .att (basic DiD) or .overall_att (staggered estimators)."
274+
"# Use .att (basic DiD; also a read-only flat-alias on staggered\n"
275+
"# classes) or .overall_att (canonical name on staggered results)."
275276
),
276277
priority="medium",
277278
step_name="robustness",

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`, `TripleDifferenceResults`, `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.
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`, `TripleDifferenceResults`, `HeterogeneousAdoptionDiDResults`); `overall_*` (`CallawaySantAnnaResults` and the rest of the staggered family); `overall_att_*` (`ContinuousDiDResults`, where `att` and `acrt` are parallel response curves); and `avg_*` (`MultiPeriodDiDResults`). Result classes in the prefixed `overall_*` / `overall_att_*` / `avg_*` families additionally expose the flat `att` / `se` / `conf_int` / `p_value` / `t_stat` names as read-only `@property` aliases over their canonical fields, for adapter / external-consumer compatibility (see PR for v3.3.3, motivated by `balance.interop.diff_diff`). The flat-native classes (`DiDResults`, `SyntheticDiDResults`, `TROPResults`, `TripleDifferenceResults`, `HeterogeneousAdoptionDiDResults`) already carry these names as native dataclass fields and are unchanged by this contract. `ContinuousDiDResults` further exposes `overall_*` aliases pointing at the ATT side (so `result.overall_se` reads `result.overall_att_se`, etc.). 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_result_aliases.py

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -60,24 +60,26 @@ def _required_init_kwargs(cls, overrides):
6060
Lets us build a minimal result instance for alias-mechanic tests without
6161
having to enumerate every estimator-specific field. Sentinel values for
6262
untouched fields are deliberately uninteresting (empty containers, zeros)
63-
-- they are not exercised by these tests."""
63+
-- they are not exercised by these tests.
64+
"""
65+
import dataclasses as _dc
66+
6467
kwargs = {}
6568
for f in fields(cls):
6669
if f.name in overrides:
6770
continue
68-
# Skip fields with defaults; we only need to fill required positionals.
69-
if f.default is not f.default_factory and f.default is not getattr(
70-
__import__("dataclasses"), "MISSING", None
71-
):
72-
# Field has a default value; let the dataclass apply it.
71+
# A field is REQUIRED iff both default and default_factory are MISSING.
72+
# When default_factory is set (e.g. list/dict factory), the dataclass
73+
# will apply it; we must NOT pre-fill the field with a sentinel or we
74+
# block the factory.
75+
if f.default is not _dc.MISSING or f.default_factory is not _dc.MISSING:
7376
continue
7477
# Required field — supply a type-compatible sentinel.
78+
# Order container annotations BEFORE the scalar `"float"` / `"int"`
79+
# branches so that ``Tuple[float, float]`` is not mis-classified as
80+
# scalar (``"float" in "Tuple[float, float]"`` is True).
7581
ann = str(f.type)
76-
if "float" in ann:
77-
kwargs[f.name] = 0.0
78-
elif "int" in ann:
79-
kwargs[f.name] = 0
80-
elif "Tuple" in ann or "tuple" in ann:
82+
if "Tuple" in ann or "tuple" in ann:
8183
kwargs[f.name] = (0.0, 0.0)
8284
elif "List" in ann or "list" in ann:
8385
kwargs[f.name] = []
@@ -87,6 +89,10 @@ def _required_init_kwargs(cls, overrides):
8789
kwargs[f.name] = pd.DataFrame()
8890
elif "ndarray" in ann or "np.ndarray" in ann:
8991
kwargs[f.name] = np.array([])
92+
elif "float" in ann:
93+
kwargs[f.name] = 0.0
94+
elif "int" in ann:
95+
kwargs[f.name] = 0
9096
else:
9197
kwargs[f.name] = None
9298
kwargs.update(overrides)
@@ -304,6 +310,15 @@ def test_aliases_are_read_only(cls, ovr):
304310
for name in ("att", "se", "conf_int", "p_value", "t_stat"):
305311
with pytest.raises(AttributeError):
306312
setattr(res, name, object())
313+
# ContinuousDiDResults also exposes overall_se / overall_conf_int /
314+
# overall_p_value / overall_t_stat as read-only aliases over the
315+
# ATT-side canonical fields (no parallel `overall_att` alias is needed
316+
# because `overall_att_att` would be confusing; the flat `att` covers
317+
# that one). These must also reject assignment.
318+
if cls.__name__ == "ContinuousDiDResults":
319+
for name in ("overall_se", "overall_conf_int", "overall_p_value", "overall_t_stat"):
320+
with pytest.raises(AttributeError):
321+
setattr(res, name, object())
307322

308323

309324
@pytest.mark.parametrize(

0 commit comments

Comments
 (0)