Add inference-field aliases on staggered result classes#406
Conversation
Adds read-only @Property aliases (att / se / conf_int / p_value / t_stat) on every result class that previously only carried prefixed canonical fields, so external adapters that read getattr(res, "se") get populated values transparently. Coverage: - Pattern B (overall_*): CallawaySantAnna, Stacked, EfficientDiD, dCDH, StaggeredTripleDiff, Wooldridge, SunAbraham, ImputationDiD, TwoStage - Pattern C (overall_att_*, ATT-side headline): ContinuousDiD — adds both flat aliases and overall_* aliases for naming consistency - Pattern D (avg_*): MultiPeriodDiD Aliases are pure read-throughs over canonical fields — no recomputation, no behavior change. NaN-canonical → NaN-alias inheritance is regression- locked at tests/test_result_aliases.py::test_pattern_b_aliases_propagate_nan. The native overall_* / overall_att_* / avg_* names remain canonical for documentation and computation. Motivated by an external adapter that reads getattr(res, "se", None) without a fallback to overall_se / overall_att_se. Pre-alias every staggered result class returned None on those keys; aliases fix the adapter's diagnostic surface transparently with no consumer-side change. 23 alias-mechanic + adapter-pattern regression tests at tests/test_result_aliases.py. Documented in CHANGELOG (Unreleased) and REGISTRY.md preamble. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
MethodologyNo P0/P1/P2 findings. Severity: P3 informational Code QualityNo findings. The aliases are simple read-only properties and do not introduce new inference logic. PerformanceNo findings. Property access is constant-time and does not add material overhead. MaintainabilityNo blocking findings. Sibling-surface mirror audit checked the parallel result naming surfaces: Tech DebtNo findings. I did not see new untracked correctness debt, and no TODO mitigation is needed for this PR. SecurityNo findings. Secret-pattern grep over the changed files found no obvious accidental credentials. Documentation/TestsSeverity: P3 informational Validation note: attempted |
- Three -> four (the preamble enumerates four native naming patterns: flat, overall_*, overall_att_*, avg_*) - Tighten "Every result class" -> "Every scalar treatment-effect result class covered by this naming contract" so the alias contract is explicitly scoped to scalar headline classes (excludes per-cell types like GroupTimeEffect / PeriodEffect that have flat fields natively) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/ai-review |
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
MethodologyNo P0/P1/P2 findings. Severity: P3 informational Code QualityNo findings. The implementation is simple property delegation and avoids new inference logic. PerformanceNo findings. Property access is constant-time and adds no material runtime cost. MaintainabilityNo findings. The registry contract and tests mirror the three changed prefixed surfaces: Pattern B Tech DebtNo findings. I did not identify new untracked deferred correctness debt. SecurityNo findings. Secret-pattern grep over the changed files did not find obvious credentials. Documentation/TestsNo blocking findings. Severity: P3 informational |
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>
|
/ai-review |
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
MethodologySeverity: P3 informational Code QualityNo findings. PerformanceNo findings. Property lookup is constant-time and adds no estimator runtime cost. MaintainabilityNo findings. The registry contract and tests cover the three sibling naming surfaces: Pattern B at Tech DebtNo findings. I did not identify new untracked deferred correctness debt. SecurityNo findings. Secret-pattern grep over changed areas did not reveal credentials. Documentation/TestsSeverity: P3 informational |
Summary
@propertyaliases (att/se/conf_int/p_value/t_stat) on every staggered result class that previously only carried prefixed canonical fields (overall_*,overall_att_*,avg_*).getattr(res, "se", None)andgetattr(res, "conf_int", None)returnedNoneon every staggered result class — silently droppingse/conf_intfrom any external adapter that read them via flatgetattr.overall_*/overall_att_*/avg_*fields remain canonical).Methodology references (required if estimator / math changes)
safe_inference()joint-NaN consistency contract (perCLAUDE.md"Inference computation") is inherited automatically because aliases never recompute — NaN canonical → NaN alias is regression-locked.Validation
tests/test_result_aliases.py(23 tests: 9 Pattern-B alias-mechanics + 9 Pattern-B NaN-propagation + 3 ContinuousDiD + 1 MultiPeriod + 1 balance-adapter regression)CallawaySantAnnaResultsfit pre-PR (returnedse=None/conf_int=None) and post-PR (returns populated values).test_chaisemartin_dhaultfoeuille.py/test_staggered.py/test_continuous_did.py) all green; full per-estimator sweep (1128 tests) green pre-rebase.Security / privacy
Coverage map
overall_*→ flat (5 props each)overall_att_*→ flat +overall_*(9 props)overall_acrt_*)avg_*→ flat (5 props)Per-result-class fields enumerated in
docs/methodology/REGISTRY.mdpreamble.Generated with Claude Code