Skip to content

refactor(v4): shared BaseEstimator mixin - transactional set_params library-wide (2(c)-i) - #741

Merged
igerber merged 1 commit into
mainfrom
feat/base-estimator-mixin
Aug 1, 2026
Merged

refactor(v4): shared BaseEstimator mixin - transactional set_params library-wide (2(c)-i)#741
igerber merged 1 commit into
mainfrom
feat/base-estimator-mixin

Conversation

@igerber

@igerber igerber commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • New underscore-private leaf module diff_diff/_base.py: the shared BaseEstimator mixin replaces the 25 hand-rolled get_params/set_params pairs (6 divergent implementation shapes, 12 non-transactional) library-wide - v4-design §9 item 3 / 2(c)-i, front-loaded so the 2(c)-ii renames build on the transactional contract. The exhaustive inventory found 25 defining classes, correcting the design doc's count of 24.
  • get_params introspects the __init__ signature (per-class cache; deep= accepted uniformly - previously 4/25) so the parameter surface can never drift from the constructor. set_params is transactional via probe re-init: strict unknown-key gate, an overridable _normalize_set_params hook, then type(self)(**merged) validates before any mutation and the probe's parameter + declared derived-config attrs are adopted. Fitted state is never touched.
  • Per-class accommodations are declarative: _PARAM_ATTR_ALIASES (DiD raw vcov_type at _vcov_type_arg; PreTrendsPower powertarget_power), _DERIVED_CONFIG_ATTRS (DiD vcov trio; _vcov_type_explicit on CS/SunAbraham/WooldridgeDiD), DiD's 4-line hook for the pinned robust=-alone re-derivation, and SyntheticDiD __init__ raw-arg storage (deprecated lambda_reg/zeta normalized to None; get_params now mirrors the full signature).
  • Behavior changes, all fail-loudly (CHANGELOG has the user-facing inventory): ten estimators whose set_params accepted constructor-rejected values until fit() now raise at set_params; BaconDecomposition no longer silently ignores unknown keys; unknown-key rejection is uniformly strict; HonestDiD gains rollback, SpilloverDiD batch atomicity; TROP empty-grid and ContinuousDiD/PreTrendsPower list-normalization edge cases aligned with their constructors; get_params key order follows __init__.
  • New cross-estimator contract suite tests/test_base_estimator.py on a dynamically discovered roster (every exported fit-class must mix BaseEstimator in; LinearRegression is the one reasoned exclusion); eight lazy-validation test pins flipped to eager expectations; premerge_scan.py Check C now fires only for hand-rolled in-body get_params, with fixtures both ways.
  • Docs: CLAUDE.md inheritance map rewritten around the mixin; CONTRIBUTING.md new-parameter checklist reworked (TWFE/MPD inherit automatically, SyntheticDiD must mirror or reject explicitly); REGISTRY's EfficientDiD eager-validation deviation Note retired in favor of the library-wide contract; ten docs/api pages gain :inherited-members: so the relocated methods stay rendered; DEFERRED.md decision rows (DCDH clone-identity won't-fix; scikit-learn stays out of dev deps) and a TODO.md follow-up row; v4-design §7/§9 marked shipped.

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - no methodology changes. Configuration API only: no estimand, weighting, identification, variance, SE, or inference computation is touched.
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None. The one prior documented deviation in this area (REGISTRY's EfficientDiD eager-vs-lazy set_params split) is retired because eager transactional validation is now the uniform library-wide contract (REGISTRY Note updated in this diff).

Validation

  • Tests added/updated: tests/test_base_estimator.py (new, 209 passing checks over the dynamic roster), tests/test_premerge_scan.py (Check C fixtures both ways), lazy-validation pin flips in tests/test_staggered.py, tests/test_triple_diff.py, tests/test_two_stage.py, tests/test_imputation.py, tests/test_methodology_imputation.py; message-standardization updates in tests/test_had.py, tests/test_continuous_did.py; tests/test_methodology_sdid.py get_params key-set update. Full param/atomicity suites run green across all 25 estimators (had, rdd, changes_in_changes, estimators_vcov_type, conley_vcov, methodology_synthetic_control, methodology_sdid, wooldridge, lpdid, sun_abraham, stacked_did, spillover, bacon, pretrends, honest_did, efficient_did, chaisemartin_dhaultfoeuille, staggered, staggered_triple_diff, trop, two_stage, imputation, methodology_callaway, methodology_did, estimators); naming guard, v4 matrix, doc-deps integrity, and docs-IA suites green; docs build spot-check confirms get_params/set_params render on the pages that gained :inherited-members:.
  • Backtest / simulation / notebook evidence (if applicable): N/A - no numerical output changes.

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

… transactional probe re-init set_params library-wide (2(c)-i)

diff_diff/_base.py replaces the 25 hand-rolled get_params/set_params pairs
(the exhaustive inventory found 25 defining classes, correcting v4-design's
count of 24; changes_in_changes.py holds two) with one leaf-module mixin:

- get_params introspects the __init__ signature (per-class cls.__dict__
  cache; KEYWORD_ONLY accepted for SpilloverDiD's all-keyword signature;
  VAR_POSITIONAL/VAR_KEYWORD fail loudly) and uniformly accepts deep= -
  previously 4/25 - so every estimator's surface is signature-compatible
  with sklearn.base.clone and can never drift from its constructor.
- set_params is TRANSACTIONAL via probe re-init: strict unknown-key gate
  ("Unknown parameter: {key}" - method names and private attributes now
  raise everywhere, replacing 17 permissive hasattr gates and one silent
  no-op), then an overridable _normalize_set_params hook, then
  type(self)(**merged) which raises before self is touched, then adoption
  of parameter attrs plus declared derived-config attrs from the probe.
  Validation is exactly __init__'s validation, eagerly and atomically, on
  all 25 classes (previously 6 divergent shapes, 12 non-transactional).
- Per-class accommodations are declarative: _PARAM_ATTR_ALIASES
  (DifferenceInDifferences raw vcov_type at _vcov_type_arg, PreTrendsPower
  power->target_power), _DERIVED_CONFIG_ATTRS (DiD vcov-resolution trio;
  _vcov_type_explicit on CallawaySantAnna/SunAbraham/WooldridgeDiD), and
  DiD's 4-line _normalize_set_params override reproducing the pinned
  robust=-alone re-derivation. SyntheticDiD's __init__ now stores its raw
  conley_metric/conley_kernel (None in any legal construction) and
  normalizes the deprecated lambda_reg/zeta to None, so the probe emits
  the pinned DeprecationWarning exactly once per call that passes them and
  get_params mirrors the full signature (the old hand-rolled dict omitted
  lambda_reg/zeta and hardcoded six None literals).

Behavior changes (all fail-loudly; CHANGELOG has the user-facing list):
ten estimators whose set_params accepted constructor-rejected values until
fit() - CallawaySantAnna, TripleDifference, TwoStageDiD, ImputationDiD,
SunAbraham, StackedDiD, StaggeredTripleDifference, SpilloverDiD, TROP,
PreTrendsPower - now raise at set_params (their eight executable
lazy-validation test pins flipped; fit-time re-checks stay as a second
layer against direct attribute mutation, comments rewritten accordingly);
BaconDecomposition rejects unknown keys; HonestDiD gains rollback;
SpilloverDiD gains batch atomicity; TROP's set_params(lambda_*_grid=[])
now resolves to the constructor's default grid; ContinuousDiD.dvals /
PreTrendsPower.violation_weights list updates store the
constructor-normalized ndarray; get_params key order follows __init__.
Configured DCDH clone-identity failure (pre-existing paths_of_interest
canonicalization) and the sklearn-out-of-dev-deps posture are recorded in
DEFERRED.md; the never-had-the-pair classes are a TODO.md follow-up.

tests/test_base_estimator.py pins the contract on a DYNAMICALLY discovered
roster (every __all__ class with fit must mix BaseEstimator in;
LinearRegression is the one reasoned exclusion): signature<->get_params
sync, re-instantiation round-trip, strict-gate + batch atomicity,
value-level rollback (per-class bad-value table), deep= triple-equality,
post-normalization set_params==re-init equivalence, fitted-state
preservation, and sklearn clone under importorskip. premerge_scan's
Check C now fires only for hand-rolled in-body get_params (a base-name
whitelist cannot see SyntheticDiD's indirect inheritance; the contract
suite is the stronger replacement), with fixtures both ways.

Docs: CLAUDE.md inheritance map rewritten around the mixin;
CONTRIBUTING.md new-parameter checklist reworked (TWFE/MPD inherit
automatically; SyntheticDiD must mirror or reject explicitly); REGISTRY's
EfficientDiD eager-validation deviation Note retired in favor of the
library-wide contract; the two paper reviews echoing the old map updated;
ten docs/api pages gain :inherited-members: so the relocated methods stay
rendered; v4-design section 7/9 marked shipped with the corrected count.
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Overall assessment

Looks good — no unmitigated P0 or P1 findings.

Executive summary

  • No estimator math, weighting, identification, variance, SE, or inference computation changed.
  • Transactional set_params() correctly validates through constructor reinitialization.
  • Raw/derived configuration exceptions are handled explicitly and tested.
  • Eager validation is documented in the Methodology Registry.
  • No security or secret-exposure concerns found.
  • Tests could not be executed because this environment lacks pytest/numpy; static compilation and source checks passed.

Methodology

  • Severity: P3 — informational. Eager validation now applies across all 25 estimator configuration surfaces.
    • Impact: Invalid parameters fail earlier; statistical results are unchanged.
    • Concrete fix: None. This behavior is explicitly documented in docs/methodology/REGISTRY.md:L1522 and is therefore not a defect.

Code Quality

No findings. The shared implementation rejects unknown keys before mutation and adopts validated probe state transactionally (diff_diff/_base.py:L135-L169).

Performance

No findings. Probe construction adds minor configuration-time work but does not affect fitting or inference performance.

Maintainability

No findings. Constructor introspection, parameter aliases, and derived-state declarations provide a coherent extension mechanism (diff_diff/_base.py:L79-L133, diff_diff/estimators.py:L1181-L1200).

Tech Debt

  • Severity: P3 — informational. Configured dCDH instances retain a known sklearn clone identity limitation, and sklearn-only tests remain optional.
    • Impact: Limited to optional interoperability/testing; dependency-free configuration round-trips remain covered.
    • Concrete fix: None required; both decisions are tracked in DEFERRED.md:L127-L128.

Security

No findings.

Documentation/Tests

No findings. The dynamic suite covers roster completeness, signature synchronization, atomic rollback, fitted-state preservation, and clone behavior (tests/test_base_estimator.py:L167-L264).

@igerber igerber added the ready-for-ci Triggers CI test workflows label Aug 1, 2026
@igerber
igerber merged commit 0109604 into main Aug 1, 2026
39 of 40 checks passed
@igerber
igerber deleted the feat/base-estimator-mixin branch August 1, 2026 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant