ChangesInChanges absorbs QDiD via method= - M-015/M-143 (phase 3(c)) - #758
Merged
Conversation
…ase 3(c))
Last phase-3 row in the 4.0 ledger; 3.9 is now implementation-complete.
`ChangesInChanges(method="qdid")` fits the estimator the standalone `QDiD`
class owned; `method="cic"` (the default, encoding Athey-Imbens' p. 447
recommendation) is unchanged.
Nothing moves. Unlike 3(a) and 3(b), CiC and QDiD were already one
implementation: byte-identical constructors, fit bodies differing only in the
literal handed to the shared module-level `_fit_distributional`, one results
container, and `QDiDResults` already an alias. So this is an API merge with no
engine relocation, no mixin and no stacklevel threading.
ONLY THE CLASS SPELLING IS DEPRECATED, NOT THE ESTIMATOR. `method="qdid"` is a
supported mode and emits no advisory warning: the fit-time footnote-21
non-monotonicity UserWarning and practitioner.py's "Prefer ChangesInChanges
over QDiD" step already carry the paper's caution where a user can act on it,
and a third warning would blur which of the class/method is dying.
Validation needs BOTH halves, which is easy to get wrong. `__init__` builds its
validation dict literally rather than forwarding `get_params()`, so `method`
is added there AND read as `params.get("method", "cic")` inside
`_validate_all_params`. The `.get` default is not defensive padding: QDiD keeps
its frozen five-param `__init__`, so its `get_params()` has no "method" key and
`_fit_distributional` re-validates through that same function on every QDiD
fit - a bare `params["method"]` would KeyError there. With only the `.get`
default and no dict entry, `ChangesInChanges(method="bogus")` and the
set_params probe re-init would both succeed silently. `_validate_method` checks
isinstance BEFORE membership, matching the module's sibling validators: a bare
membership test lets `np.array(["cic"])` through (elementwise compare, 1-element
bool) and stores the ARRAY as the method tag, which is unhashable and breaks
the `_ESTIMATOR_TITLES` lookup in `summary()`.
`_fit_distributional`'s `estimator_name` now derives from `type(est).__name__`
instead of the `kind` literal, so a `method="qdid"` fit no longer raises
covariate errors naming a class the caller never constructed. Byte-identical on
both 3.x surfaces.
Results field renamed `estimator` -> `method` [M-143], the section-8 rule-9
mirror: the field already held exactly the tag the new param sets, and
`WooldridgeDiDResults.method` is the in-repo template. It also resolves a
standing collision - `AggregationResult.estimator` holds a CLASS NAME while
this one holds a method tag. Shim is `deprecated_field_property` (read-only by
design, so a stale `setattr` fails loudly rather than writing a shadow
attribute the renamed field cannot see) plus a `__setstate__` key migration;
`to_dict()` emits BOTH keys through 3.9 per the M-094 twin, so
`test_to_dict_keys` stays green untouched and the old key drops at 4.0.
The window's SCOPE is a recorded decision (REGISTRY note, cross-linked from
M-143): it covers the READ path and pickle migration, not construction, so
`ChangesInChangesResults(..., estimator=...)` raises `TypeError` from 3.9
rather than warning until 4.0. That matches both prior results-field renames -
M-094 (`treatment_col` -> `takeup`) and M-114 (`groups` -> `units`) - neither of
which preserved a deprecated constructor keyword; the container has one library
call site, and a constructor shim would add a deprecated surface with no ledger
row. Pinned by a test so the documented contract cannot drift either way.
Adding that row arms two guard duties, the first non-obvious:
`test_predicate_binds_to_ledger_tokens` forces every live field-rename token
into `_PATTERN_TOKENS`, which turns on Duty A repo-wide. Hence four
SURFACE_ALLOWLIST entries for independent same-named surfaces
(AggregationResult.estimator; the estimator= INSTANCE param on the three power
entry points) and eight CONSUMER_ALLOWLIST entries, both reconciled against
live sweeps rather than a written table. `estimator` also joins
_AMBIGUOUS_TOKENS so Duty C matches through the attr/quoted lanes only.
practitioner.py stops handing readers deprecated code: `_cic_fit_snippet` drops
`est_name` for a keyword-only `method=` and always emits `ChangesInChanges`.
Import lines are per-site, not a blanket rewrite - Step-8's imports TWO names
because `_did_anchor_snippet` appends a `DifferenceInDifferences()` call with
no import of its own, so collapsing it to one name would emit a snippet that
NameErrors. The field reads use a `__dict__` fallback rather than `getattr` for
the old name: on a real results object that name is now a deprecation property,
so reading it warns even when it returns the right value, which collides with
the warning-as-error gates.
Tests: tests/test_v4_merge_cic.py, seven gates (parity, committed pre-merge
oracle, validation, deprecation, field shim, engine guards, consumers). Parity
is bit-exact - same engine, same process, so a needed tolerance IS the finding.
The oracle is captured from the UNMODIFIED tree by
tests/_capture_v4_merge_cic_oracles.py; it covers the unconditional arm only
(the covariate QR path is tie-selection-bounded with BLAS-dependent tie flips
and CI spans four platforms) and deliberately omits quantile_effects, which
benchmarks/data/qte_golden.json already pins against R qte 1.3.1 at
atol=1e-10 - a stronger cross-tree pin, and the reason 3(b)'s oracle doctrine
does not transfer wholesale. The consumer gate EXECS the emitted snippet rather
than compiling it, since a missing import is invisible to compile().
Also records, in docs/dev-status.md, that local `mypy` can fail for an
environment reason unrelated to any code defect: lint.yml pins numpy==2.4.5 but
that pin lives only in the workflow, so a drifted local numpy (2.5.1) makes
mypy abort inside numpy's own stubs and stop checking entirely.
Verified: 63/63 new gates; naming guard 20/20; matrix/base-estimator/
inference-policy/serialization 718; CiC family + drift + carousel 289; docs
gates 373; notebook executes under the diffdiff-dev kernel with a 5-line diff;
Sphinx -W build succeeded with zero warnings; ruff, black, and mypy clean at
the CI-pinned toolchain.
The regenerated ChangesInChangesResults autosummary stub is committed because
this change genuinely altered the API shape (`estimator` is now a property, not
a dataclass field). The two unrelated stubs the build also rewrote were
reverted - that staleness predates this branch.
Overall assessment✅ Looks good — no unmitigated P0/P1 findings. Executive summary
MethodologyNo findings. The affected methods are Changes-in-Changes and QDiD. Dispatch remains on the shared engine and agrees with the documented behavior in Code QualityNo findings. PerformanceNo findings. Dispatch adds negligible overhead and does not alter estimator complexity. MaintainabilityNo findings. Validation, pickle migration, and the deprecated read-only alias are centralized and tested. Tech DebtNo findings. Existing methodology limitations remain tracked in SecurityNo findings. No apparent secrets or unsafe behavior were introduced. Documentation/TestsP2 — Existing addition is now under the Deprecated heading
P3 — Fit docstring describes covariates as CiC-only
Verification was static because the review environment lacks |
…ates wording CI review, both non-blocking. P2: the new '### Deprecated' heading swallowed the pre-existing Phase 3(b) 'TripleDifference serves both DDD designs' entry, rendering an Added item as a deprecation. Restores an '### Added' before it - the Unreleased block already repeats headings per wave, so this matches the file's own convention. P3: fit()'s covariates entry described the branch as 'conditional (quantile-regression) CiC', implying covariates do not apply to method='qdid'. They do; only the cells regressed differ (CiC two control cells, QDiD three). The class docstring was already made method-aware - this is its fit-level twin, which the original sweep missed. Sphinx -W still builds with zero warnings (the edit stays inside the numpydoc Parameters block's indentation, the exact shape that broke in 3(b)).
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0/P1 findings. Executive summary
MethodologyNo findings. The affected methods are Changes-in-Changes and QDiD. Dispatch matches the documented mode-specific behavior in Code QualityNo findings. PerformanceNo findings. The dispatch adds negligible overhead. MaintainabilityNo findings. Validation and legacy result-field migration are centralized and tested. Tech DebtNo findings. Existing methodological limitations remain documented and tracked. SecurityNo findings. No secrets or unsafe behavior were identified. Documentation/TestsNo findings.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 3(c) of the 4.0 program — the last non-terminal phase-3 row, so 3.9 is now implementation-complete and what follows is Phase 4 (migration guide, version bump, cut).
ChangesInChanges(method="qdid")fits the estimator the standaloneQDiDclass owned;method="cic"(the default, encoding Athey & Imbens' p. 447 recommendation) is unchanged.method=is keyword-only, so every existing positional slot is preserved, and lowercase-only._fit_distributional, one results container, andQDiDResultsalready an alias. No engine relocation, no mixin, nostacklevelthreading.method="qdid"is fully supported and emits no advisory warning — the fit-time footnote-21 non-monotonicityUserWarningandpractitioner.py's "Prefer ChangesInChanges over QDiD" step already carry the caution where a user can act on it, and a third warning would blur which of the class/method is dying.QDiDwarns on construction (FutureWarning) and is removed in 4.0 [M-015]; theQDiDResultsalias dies with it [M-061].estimator→method[M-143], the section-8 rule-9 mirror. Read-only deprecation property,__setstate__pickle migration, and a dual-keyto_dict()window through 3.9.method="qdid"fit now nameChangesInChangesrather than a class the caller never constructed (estimator_namederives fromtype(est).__name__).practitioner.pyno longer emitsQDiD(...)in generated guidance.Two things reviewers may want to look at directly
Validation lives in two places on purpose.
__init__builds its validation dict literally rather than forwardingget_params(), somethodis added there and read asparams.get("method", "cic")inside_validate_all_params. The.getdefault is not padding:QDiDkeeps its frozen five-param__init__, so itsget_params()has no"method"key and_fit_distributionalre-validates through that same function on every QDiD fit — a bareparams["method"]wouldKeyErrorthere. With only the.getdefault and no dict entry,ChangesInChanges(method="bogus")and theset_paramsprobe re-init would both succeed silently.The deprecation window's scope is a recorded decision (REGISTRY note, cross-linked from M-143): it covers the READ path and pickle migration, not construction, so
ChangesInChangesResults(..., estimator=...)raisesTypeErrorfrom 3.9 rather than warning until 4.0. That matches both prior results-field renames — M-094 (treatment_col→takeup) and M-114 (groups→units) — neither of which preserved a deprecated constructor keyword. The container has one library call site, and a constructor shim would add a deprecated surface carrying no ledger row. Pinned by a test so the documented contract cannot drift either way.Methodology references
qtev1.3.1 (qte::CiC()/qte::QDiD()).- **Note:**entries: (1)method="qdid"deliberately emits no advisory warning, with the reasoning above; (2) the M-143 deprecation window covers reads and pickles but not construction, matching M-094/M-114. Mode-specific behaviour that the shared signature can hide is documented explicitly — the eq. 17 interior-range guard andq_lower/q_upperare CiC-only (NaN for everyqdidfit), and the covariate branch fits QRs in two control cells for CiC but three for QDiD.Validation
tests/test_v4_merge_cic.py(63 gates across parity, committed pre-merge oracle, validation, deprecation, field shim, engine guards, consumers) andtests/_capture_v4_merge_cic_oracles.py(the capture script).assert_array_equal) — same engine, same process, so a needed tolerance would itself be the finding.c2941caaunderDIFF_DIFF_BACKEND=python. It covers the unconditional arm only (the covariate QR path is tie-selection-bounded with BLAS-dependent tie flips and CI spans four platforms) and deliberately omitsquantile_effects, whichbenchmarks/data/qte_golden.jsonalready pins against Rqte1.3.1 atatol=1e-10— a stronger cross-tree pin.NameErrorinvisible tocompile().test_practitioner.py(7 mock sites →method=),test_t27_cic_distributional_effects_drift.py(migrated off the deprecated class; itssimplefilter("error")block would otherwise defeat the pyproject filter),test_base_estimator.py,test_naming_guard.py,test_v4_matrix.py.docs/tutorials/27_cic_distributional_effects.ipynbmigrated off the deprecated class (import in cell 2, constructions in cell 23, stored practitioner output in cell 29) and executed under thediffdiff-devkernel — 5-line diff, no output churn.-Wbuild succeeded with zero warnings;ruff,black, andmypyclean at the CI-pinned toolchain.Security / privacy
tests/test_naming_guard.py, verified as a false positive — the patterntoken[[:space:]]*[=:]matched the literal phrase "M-143's old token:" in a code comment. No credential material is involved.