Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions .claude/scripts/premerge_scan.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,17 +111,24 @@ def _init_self_assigns(cls: ast.ClassDef) -> "set[str]":
return names


def _get_params_refs(cls: ast.ClassDef) -> "set[str]":
refs: "set[str]" = set()
def _own_get_params(cls: ast.ClassDef) -> "ast.FunctionDef | None":
for node in cls.body:
if isinstance(node, ast.FunctionDef) and node.name == "get_params":
for sub in ast.walk(node):
if isinstance(sub, ast.Attribute):
refs.add(sub.attr)
elif isinstance(sub, ast.Name):
refs.add(sub.id)
elif isinstance(sub, ast.Constant) and isinstance(sub.value, str):
refs.add(sub.value)
return node
return None


def _get_params_refs(cls: ast.ClassDef) -> "set[str]":
refs: "set[str]" = set()
node = _own_get_params(cls)
if node is not None:
for sub in ast.walk(node):
if isinstance(sub, ast.Attribute):
refs.add(sub.attr)
elif isinstance(sub, ast.Name):
refs.add(sub.id)
elif isinstance(sub, ast.Constant) and isinstance(sub.value, str):
refs.add(sub.value)
return refs


Expand All @@ -130,15 +137,23 @@ def new_params_missing_from_get_params(
) -> "list[str]":
"""Check C (AST-based): of the ``added_param_names`` (new ``self.X`` from the diff),
return those that are assigned in some class ``__init__`` but never referenced in
that same class's ``get_params()``. Restricting to ``__init__`` and to the class's
own ``get_params`` avoids the old substring heuristic's false negatives."""
that same class's OWN in-body ``get_params()``. Classes WITHOUT an in-body
``get_params`` are skipped entirely: post-mixin, estimators inherit an
introspective ``get_params`` from ``diff_diff._base.BaseEstimator`` (possibly
indirectly, e.g. SyntheticDiD via DifferenceInDifferences - invisible to this
file-local AST scan), which derives its keys from the ``__init__`` signature,
so a new constructor param is auto-covered and the init-signature<->get_params
sync is enforced by ``tests/test_base_estimator.py``. The check remains for
any class that still hand-rolls ``get_params``."""
try:
tree = ast.parse(file_text)
except SyntaxError:
return []
added = set(added_param_names)
missing: "set[str]" = set()
for cls in (n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)):
if _own_get_params(cls) is None:
continue
assigned = _init_self_assigns(cls) & added
if not assigned:
continue
Expand Down
36 changes: 36 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,42 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed
- **Shared `BaseEstimator` mixin** (`diff_diff/_base.py`, v4 program 2(c)-i):
the 25 hand-rolled `get_params`/`set_params` pairs (6 divergent
implementation shapes) are replaced by one introspective mixin.
`get_params` derives its keys from the `__init__` signature and uniformly
accepts `deep=` (previously 4/25); `set_params` is TRANSACTIONAL via probe
re-init - it validates by constructing `type(self)(**merged)` before any
mutation, so it enforces exactly the constructor's validation, eagerly and
atomically, on every estimator. Behavior changes, all in the
fail-loudly direction: (1) ten estimators whose `set_params` previously
accepted constructor-rejected values until `fit()` (CallawaySantAnna,
TripleDifference, TwoStageDiD, ImputationDiD, SunAbraham, StackedDiD,
StaggeredTripleDifference, SpilloverDiD, TROP, PreTrendsPower) now raise at
`set_params`; (2) `BaconDecomposition.set_params` no longer silently
ignores unknown keys; (3) unknown-key rejection is uniformly strict
(method names / private attributes raise `ValueError: Unknown parameter:
...`) where 17 classes previously accepted any existing attribute name;
(4) `HonestDiD` gains rollback (a failed call previously left the instance
part-mutated) and `SpilloverDiD` gains batch atomicity; (5)
`TROP.set_params(lambda_*_grid=[])` now resolves the empty grid to the
constructor's default grid (matching `__init__`; the raw-`[]` state was
reachable only through the old setter); (6) list-valued `set_params`
updates to `ContinuousDiD.dvals` / `PreTrendsPower.violation_weights` now
store the constructor-normalized ndarray; (7) `SyntheticDiD.get_params()`
now mirrors its full `__init__` signature - the deprecated
`lambda_reg`/`zeta` appear as always-`None` keys and round-trip silently;
(8) `get_params` key order follows the `__init__` signature everywhere.
Estimator `get_params`/`set_params` signatures are sklearn-compatible
library-wide, so `sklearn.base.clone` round-trips (where scikit-learn is
installed; it is deliberately not a dependency) - except configured
`ChaisemartinDHaultfoeuille` instances, whose pre-existing
`paths_of_interest` canonicalization fails clone's parameter-identity
check (recorded in DEFERRED.md). The cross-estimator contract is pinned by
`tests/test_base_estimator.py` on a dynamically discovered roster; the
pre-merge scanner's Check C now covers hand-rolled `get_params` only.

### Added
- **Naming-completeness guard test** (`tests/test_naming_guard.py`, internal):
mechanical enforcement of the three v4-program naming invariants that were
Expand Down
33 changes: 17 additions & 16 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,23 +83,24 @@ pytest tests/test_rust_backend.py -v
together using `safe_inference()` from `diff_diff.utils`. Never compute individually.
7. **Estimator inheritance** — understanding this prevents consistency bugs:
```
DifferenceInDifferences (base class)
├── TwoWayFixedEffects (inherits get_params/set_params)
└── MultiPeriodDiD (inherits get_params/set_params)

Standalone estimators (each has own get_params/set_params):
├── CallawaySantAnna
├── SunAbraham
├── ImputationDiD
├── TwoStageDiD
├── TripleDifference
├── TROP
├── StackedDiD
├── SyntheticDiD
└── BaconDecomposition
BaseEstimator (diff_diff/_base.py — shared get_params/set_params mixin)
├── DifferenceInDifferences
│ ├── TwoWayFixedEffects
│ ├── MultiPeriodDiD
│ └── SyntheticDiD
└── every other estimator class (CallawaySantAnna, SunAbraham,
ImputationDiD, TwoStageDiD, TripleDifference, TROP, StackedDiD,
BaconDecomposition, ... — 25 classes total)
```
When adding params to `DifferenceInDifferences.get_params()`, subclasses inherit automatically.
Standalone estimators must be updated individually.
`get_params` introspects the `__init__` signature and `set_params` is
TRANSACTIONAL via probe re-init (`type(self)(**merged)` validates before
any mutation) — so a new `__init__` param is automatically in
`get_params`/`set_params` for every class, and `set_params` enforces
exactly the constructor's validation, eagerly. Per-class accommodations
are declarative class attrs (`_PARAM_ATTR_ALIASES`,
`_DERIVED_CONFIG_ATTRS`, `_normalize_set_params`); the cross-estimator
contract is pinned by `tests/test_base_estimator.py` (dynamic roster —
new estimators are automatically enrolled and must mix BaseEstimator in).
8. **Dependencies**: numpy, pandas, and scipy ONLY. No statsmodels.

## Documenting Deviations (AI Review Compatibility)
Expand Down
19 changes: 15 additions & 4 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,24 @@ grep -rn "self\.<param>" diff_diff/<module>.py

A new parameter is only complete when it is:

- stored on `self` and returned by `get_params()` (and accepted by `set_params()`)
- stored on `self` under its own name — `get_params()`/`set_params()` then
cover it automatically (the shared `BaseEstimator` mixin introspects the
`__init__` signature; a param stored under a DIFFERENT attribute name needs
a `_PARAM_ATTR_ALIASES` entry, and constructor-derived state a
`_DERIVED_CONFIG_ATTRS` entry — see `diff_diff/_base.py` and the
inheritance map in `CLAUDE.md`)
- applied in **every** aggregation mode — `simple`, `event_study`, and `group`
- applied in the **bootstrap/inference** paths, not just the analytical one
- reflected on the result object, so `to_dict()`/`summary()` do not misreport it
- propagated to the estimators that inherit it (see the inheritance map in
`CLAUDE.md` — subclasses of `DifferenceInDifferences` inherit automatically,
standalone estimators must each be updated)
- propagated to the estimators that inherit it: `TwoWayFixedEffects` and
`MultiPeriodDiD` define no `__init__` of their own, so they inherit a new
`DifferenceInDifferences` constructor parameter automatically;
`SyntheticDiD` defines its OWN signature (it forwards only
`robust`/`cluster`/`alpha` to `super().__init__`), so a new parent
parameter must be explicitly mirrored — or explicitly rejected, like its
`vcov_type`/`conley_*` guards — there, and `BaseEstimator` introspects the
concrete signature, so an unmirrored parent param simply won't appear in
`SyntheticDiD.get_params()`

The recurring bug is a parameter that works in the default aggregation and is
silently ignored in one of the others. The same trace applies when adding a new
Expand Down
2 changes: 2 additions & 0 deletions DEFERRED.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ decisions (refactor waivers, perf trade-offs, test-infrastructure calls) are rec

| Decision | Location | Verified |
|----------|----------|----------|
| **DCDH `sklearn.base.clone` param-identity failure won't-fix.** `ChaisemartinDHaultfoeuille._validate_paths_of_interest` unconditionally canonicalizes `paths_of_interest` into a fresh `List[Tuple[int, ...]]`, so sklearn `clone()`'s post-construction `param1 is param2` identity check fails for configured instances - a pre-existing normalization the BaseEstimator mixin PR documented rather than changed (get_params/set_params signatures are clone-compatible; the dependency-free `cls(**est.get_params())` config-equality contract is the enforced one, `tests/test_base_estimator.py`). Fixing would mean returning the caller's raw object from a validator whose job is canonicalization. | `chaisemartin_dhaultfoeuille.py` | mixin PR / 2026-08-01 |
| **scikit-learn stays out of dev deps; clone-identity tests remain importorskip-only.** The sklearn-`clone()` round-trip tests (`test_base_estimator.py`, had/rdd/cic suites) run only where scikit-learn happens to be installed - deliberate, matching the numpy/pandas/scipy-only dependency posture; the always-running contract is the dependency-free re-instantiation config-equality test. | `tests/test_base_estimator.py` | mixin PR / 2026-08-01 |
| **Plan-review hash gate threat model: accident prevention, NOT adversarial defense.** The ExitPlanMode content-hash gate (hook + `plan_snapshot.py`) exists to stop accidents — stale approvals, concurrent-worktree cross-talk, plans edited mid-review — all of which it closes by construction (snapshot identity + invocation-unique state tokens, 30+ behavioral tests). It does NOT and cannot defend against a malicious local process: nothing verifies review AUTHORSHIP, and such an actor can simply write a matching review file directly — no userland hook can prevent that short of signed reviews, which is out of scope for a personal workflow aid. Review findings that presuppose a hostile local actor against this gate are waived by this decision (2026-07-20, after 7 local AI-review rounds converged on ever-deeper adversarial-model refinements with no reachable fixpoint). Genuine accident vectors remain in scope and are fixed as found. | `.claude/hooks/check-plan-review.py`, `.claude/scripts/plan_snapshot.py` | 2026-07-20 |
| **`StackedDiD` survey re-resolution intra-file dedup not warranted.** The cross-estimator ContinuousDiD/EfficientDiD panel-to-unit collapse consolidation landed in #226 (`ResolvedSurveyDesign.subset_to_units_by_row_idx` / `build_unit_first_row_index`); StackedDiD deliberately re-resolves at stacked granularity (control units are duplicated across sub-experiments). The residual intra-file dedup (raw-weight extraction ×3, compose-normalize ×3, resolve-on-stacked ×2) is stacked-specific, low value, and touches the numerically-sensitive composed-weight renormalization; cross-estimator unification was assessed and is not warranted (genuinely different mechanisms; all three already delegate to `_resolve_survey_for_fit` / `compute_survey_metadata`). | `stacked_did.py` | #226 review |
| **`HeterogeneousAdoptionDiD` Phase-4 Pierce-Schott (2016) replication harness waived.** R parity at `atol=1e-8` on the same 3 DGPs is a strictly stronger anchor than reproducing Fig 2's pointwise CIs on the LBD-restricted PNTR panel (paper §5.2 self-acknowledges NP estimators too noisy there). Re-open only on user demand. See REGISTRY HAD Deviations Notes #3/#4. | `benchmarks/`, `tests/` | Phase 2a / 2026-05-20 |
Expand Down
1 change: 1 addition & 0 deletions TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ generic sparse-FE, QR+SVD rank-detection redundancy, `check_finite` bypass — m

| Issue | Location | Origin | Effort | Priority |
|-------|----------|--------|--------|----------|
| Evaluate adding the `BaseEstimator` param surface (get_params/set_params) to the exported classes that never had it - `PowerAnalysis`, `LinearRegression`, `BusinessReport`, `DiagnosticReport`, `TWFEWeightsResult` (a NEW public surface, deliberately out of the 2(c)-i pure-refactor scope; `LinearRegression` is the one `fit`-bearing class excluded from the contract suite's roster-completeness test). | `diff_diff/linalg.py`, `diff_diff/power.py` | mixin PR | Mid | Low |
| Tighten the mypy suppressions that back the enforced-zero posture: burn down `prep_dgp`'s per-module `[index]` override (needs a None-vs-array restructure that preserves the seeded RNG stream), and evaluate re-enabling the globally disabled codes (`arg-type`, `return-value`, `var-annotated`, `assignment`) one at a time — `assignment` alone hid several real annotation drifts found during the 2026-07 triage. | `pyproject.toml` `[tool.mypy]`, `diff_diff/prep_dgp.py` | lint-CI | Mid | Low |
| MMM interop follow-up: Meridian `roi_calibration_period` mask builder - accept the MMM's time index + channel order and emit the boolean `(n_media_times, n_media_channels)` mask so `.to_code()` scopes the prior to the experiment window automatically (today the caller passes a mask expression / `full_model_window=True`). | `diff_diff/mmm.py` | mmm-interop | Quick | Low |
| MMM interop PR-B: calibration tutorial notebook (fit DiD/CS -> scope -> `to_pymc_marketing_lift_test` / `to_meridian_roi_prior`) + a `llms-practitioner.txt` Step 8 pointer to the exporters as the MMM hand-off. | `docs/tutorials/`, `diff_diff/guides/llms-practitioner.txt` | mmm-interop | Mid | Low |
Expand Down
Loading
Loading