TripleDifference absorbs StaggeredTripleDifference - M-013/M-064 (phase 3(b)) - #756
Merged
Conversation
…M-064 (phase 3(b)) Phase 3 of the 4.0 program collapses redundant estimator pairs into one class with two modes. `TripleDifference` now serves BOTH DDD designs from one signature, mirroring the reference implementation (Ortiz-Villavicencio & Sant'Anna's `triplediff::ddd()`, which serves both from one function). `StaggeredTripleDifference` and its `SDDD` alias are deprecated in 3.9 and removed at 4.0. The merge is API-only: the estimation core moved VERBATIM into the private `diff_diff/_staggered_triple_diff_engine.py`, mixed into both classes, so neither surface forks the math. Mechanics - `fit()` dispatches on `first_treat=`. The staggered-only fit params are keyword-only, so a positionally-written staggered call cannot bind to the 2x2x2 slots; positional slots 1-8 (the deprecated `time=` included) are preserved byte-for-byte, so nothing narrows. - Mixing the two designs' params raises in BOTH directions, constructor params included. `bootstrap_weights`/`seed`/`cband` are accepted as inert in 2x2x2 mode (unreachable without `n_bootstrap > 0`, which that mode rejects); the power entry points apply the SAME boundary, so an estimator that is legal to fit is legal to simulate. - `time=` is the calendar column in staggered mode (no rename warning) and the deprecated `post=` alias in 2x2x2 mode, resolved by dispatch order. - `cluster=` raises in staggered mode, steering to `n_bootstrap > 0`; analytical clustered SEs are not implemented for that engine. - Returns `StaggeredTripleDiffResults` through 3.9; container unification is M-014's job at 4.0. Verification that the relocation moved no numbers: oracles captured from the UNMODIFIED pre-move tree and committed before any source edit (`tests/_capture_v4_merge_ddd_oracles.py`), covering every branch that moved - the DR base config and its bootstrap, all three nuisance models under covariates, the never-treated comparison fork and the survey-pweight path. All reproduce bit-identically post-relocation. Parity between the two callers of a relocated engine cannot see a transcription slip in the move itself, which is why the absolute pins exist. TWO changes affect results and are called out in CHANGELOG: 1. Negative `first_treat` cohort values now RAISE on both surfaces. Such units belonged to neither the treated (`g > 0`) nor the never-enabled (`g == 0`) population: still counted in `n_obs`, contributing to no ATT comparison, so a fit returned a plausible finite estimate for a silently different sample (measured: overall_att 3.29438582 -> 2.99470938, n_never_enabled 24 -> 0). The `+inf` sentinel on the same parameter was already recoded-and-warned, so silence on negatives was a hole in an established contract. Affected fits now error instead of returning a number; `0`/`+inf` fits are unchanged. 2. A positive enabling cohort with no eligible treated units now warns and is excluded from `groups`/`n_groups`, which previously advertised coverage the estimate did not have. Estimates are unchanged - reporting only. Also: `anticipation` validated as a non-negative integer via a shared `utils.validate_anticipation`, applied in the constructor and in the engine so both surfaces fail closed (the deprecated class's API shape stays frozen - construction succeeds, fit raises, because this is an identification guard); `pscore_trim` gains the staggered range check plus a type guard (row M-142); the staggered-only constructor roster is centralized in `utils` so `fit()` and the power guard cannot drift apart. Ledger: M-013 shimmed, M-064 phase 5, new M-140/M-141 (fit-time `aggregate=`/`balance_e=` carve-out) and M-142; M-014/M-031/M-075/M-081 notes. Deferred work is recorded in TODO.md rather than left implicit. Unrelated drive-by, kept because this PR already edits the file: the custom-DGP cell in `06_power_analysis.ipynb` passed the deprecated `time=` alongside the profile's own `post=`, which the M-031 shim correctly rejects. Broken on main since the rename wave; CI excludes that notebook as too slow.
Overall assessmentExecutive summary
MethodologyNo unmitigated methodology defect found. The affected method is staggered
Code QualityCQ-1: Bootstrap warning names the wrong estimator
PerformanceNo performance findings. MaintainabilityM-1: Power and fit encode different staggered-mode boundaries
Tech DebtTD-1: Bootstrap seed provenance remains absent from resultsTracked in
SecurityNo security issues or accidental secrets found. Documentation/TestsNo independent documentation defect found. The P1 boundary mismatch lacks regression coverage for explicit default-valued kwargs.
Path to Approval
|
…imator in the shared bootstrap warning
Two defects, both real, both verified by execution.
1. The power guard rejected `aggregate`/`balance_e` by KEY PRESENCE, while
TripleDifference.fit() rejects them only when NON-None. So
`fit(..., aggregate=None, balance_e=None)` succeeded in 2x2x2 mode while
`estimator_kwargs={"aggregate": None}` raised at all three power entry
points - breaking the very "legal to fit implies legal to simulate"
boundary the guard exists to uphold. This was a gap in the earlier
constructor-arm alignment, which left the fit-kwargs arm on presence checks.
The two tests now differ per param because fit()'s do: `first_treat` and
`unit` are sentinel-defaulted, so SUPPLYING them at all selects staggered
mode (an explicit `first_treat=None` still does, then fails on a missing
column), while `aggregate`/`balance_e` default to None and only a non-None
value is staggered-only. Presence test for the first pair, value test for
the second.
2. The single-PSU/degenerate-design bootstrap warning hard-coded
"CallawaySantAnna bootstrap ...", but it lives in the mixin shared by
CallawaySantAnna and both DDD classes - so a DDD fit failing closed pointed
diagnosis at the wrong estimator. Now interpolates a `_BOOTSTRAP_LABEL`
ClassVar supplied by each host; CallawaySantAnna's message is byte-identical
to before.
The mixin declares the attribute as ClassVar[str], not a bare annotation:
an instance-variable declaration there makes every host's class-level
assignment a "cannot override instance variable with class variable" [misc]
error under the zero-error mypy gate. Note the contrast with
`_warn_frame_offset` in the same subsystem, which IS assigned via instance
and therefore must NOT be a ClassVar - the two have opposite correct
answers.
Tests: both power arms pinned in both directions across simulate_power /
simulate_mde / simulate_sample_size (explicit None accepted, non-None
rejected, mode selectors rejected even as None). The bootstrap-label test
routes through a real single-PSU degenerate fit on BOTH surfaces rather than
asserting on source text - a source pin cannot see whether the interpolated
value reaches the user - and was confirmed non-vacuous by re-injecting the old
literal and observing it fail.
1164 passed; mypy clean at the CI-pinned toolchain.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologyDocumented staggered-DDD deviations
No undocumented estimator, weighting, identification, or variance mismatch found. Code QualityNo findings. The prior misleading bootstrap label is corrected at PerformanceNo findings. MaintainabilityNo findings. The prior fit/power boundary mismatch is corrected by separate presence- and value-based checks at Tech DebtTracked staggered-DDD limitations
SecurityNo security issues or accidental secrets found. Documentation/TestsNo findings. Regression coverage now includes explicit- |
…eters block
The Sphinx -W build failed on three warnings in TripleDifference's docstring
(inline-literal start-string without end-string; field list ends without a
blank line; block quote ends without a blank line).
Cause: a flush-left prose paragraph sat INSIDE the numpydoc Parameters
section. napoleon parses every line there as a parameter entry, so it rewrote
the prose into bogus fields and split ``fit(..., first_treat=...)`` at the
comma, orphaning the opening backticks:
:param The remaining parameters apply to the STAGGERED mode only:
:param (``fit(...:
:param first_treat=...)``). Four of them - ``control_group``:
The other two warnings cascade from that break.
Moved the two paragraphs verbatim into the existing Notes section, which is
numpydoc's home for cross-parameter discussion. No content lost; each
parameter entry is unchanged.
Verified with a real sphinx-build -W against CI's exact dependency list:
"build succeeded", zero warnings (previously Error 1). Confirmed this was a
regression rather than pre-existing - origin/main's docstring parses clean
through the same napoleon pipeline - and swept every public docstring this
branch touches for the same defect; none others.
The build also regenerates docs/api/_autosummary/*.rst; those refreshes are
deliberately NOT committed. main's stubs already omitted the Attributes rubric
for attributes that existed there, so the staleness predates this branch, and
committing locally-regenerated stubs would add sphinx-version churn unrelated
to the fix.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologyDocumented staggered-DDD deviations
No undocumented estimator, weighting, identification, variance, or SE mismatch found. Code QualityNo findings. The estimator-specific bootstrap warning is correctly implemented at PerformanceNo findings. MaintainabilityNo findings. Fit and power mode detection share the same constructor-boundary helper, while explicit Tech DebtTracked staggered-DDD limitations
SecurityNo security issues or accidental secrets found. Documentation/TestsNo findings. The latest docstring change places cross-parameter discussion in the |
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 of the 4.0 program collapses redundant estimator pairs into one class with two modes.
TripleDifferencenow serves both DDD designs from one signature, mirroring the reference implementation (triplediff::ddd()serves both from one function).StaggeredTripleDifferenceand itsSDDDalias are deprecated in 3.9, removed at 4.0.The merge is API-only: the estimation core moved verbatim into the private
diff_diff/_staggered_triple_diff_engine.py, mixed into both classes, so neither surface forks the math.fit()dispatches onfirst_treat=. Staggered-only fit params are keyword-only, so a positionally-written staggered call cannot bind to the 2x2x2 slots; positional slots 1-8 (the deprecatedtime=included) are preserved byte-for-byte — nothing narrows.bootstrap_weights/seed/cbandare accepted as inert in 2x2x2 mode (unreachable withoutn_bootstrap > 0, which that mode rejects); the power entry points apply the same boundary, so an estimator legal tofit()is legal to simulate. That boundary is defined once inutilsand consumed by both.time=is the calendar column in staggered mode (no rename warning) and the deprecatedpost=alias in 2x2x2 mode, resolved by dispatch order rather than value inspection.cluster=raises in staggered mode, steering ton_bootstrap > 0; analytical clustered SEs are not implemented for that engine.StaggeredTripleDiffResultsthrough 3.9; container unification is M-014's job at 4.0.Two changes affect results
first_treatcohort values now raise on both surfaces. Such units belonged to neither the treated (g > 0) nor the never-enabled (g == 0) population — still counted inn_obswhile contributing to no ATT comparison — so a fit returned a plausible finite estimate for a silently different sample (measured:overall_att3.29438582 → 2.99470938,n_never_enabled24 → 0, no error or warning). The+infsentinel on the same parameter was already recoded-and-warned, so silence on negatives was a gap in an established contract. Affected fits now error instead of returning a number;0/+inffits are byte-for-byte unchanged.groups/n_groups, which previously advertised coverage the estimate did not have. Estimates unchanged — reporting only.Both were pre-existing behaviors of the relocated engine, verified against
main.Also:
anticipationvalidated as a non-negative integer via a sharedutils.validate_anticipation, applied in the constructor and in the engine so both surfaces fail closed (the deprecated class's API shape stays frozen — construction succeeds,fit()raises, because this is an identification guard, not a signature change);pscore_trimgains the staggered range check plus a type guard (row M-142).Ledger: M-013 shimmed, M-064 phase 5, new M-140/M-141 (fit-time
aggregate=/balance_e=carve-out) and M-142; M-014/M-031/M-075/M-081 notes. Deferred work is recorded inTODO.mdrather than left implicit.Unrelated drive-by, kept because this PR already edits the file: the custom-DGP cell in
06_power_analysis.ipynbpassed the deprecatedtime=alongside the profile's ownpost=, which the M-031 shim correctly rejects. Broken onmainsince the rename wave; CI excludes that notebook as too slow.Methodology references (required if estimator / math changes)
ATT(g,t); optimal-GMM combination across comparison cohorts; influence-function SEs; multiplier bootstrap.triplediff. Seedocs/methodology/REGISTRY.md§ TripleDifference — Staggered mode.g_c > max(g,t); eligible-treated cohort mass in aggregation; group-level WIF adjustment; default overall ATT vs Eq. 4.14, available asoverall_att_es) are unchanged and remain labelled in REGISTRY.md. New API decisions documented there as Notes:cluster=raising on the new surface, two result containers through 3.9, thepscore_trimtightening, the inert bootstrap satellites, thefirst_treatencoding contract, and degenerate-cohort reporting.Validation
tests/test_v4_merge_ddd.py(new, 147 gates),tests/_capture_v4_merge_ddd_oracles.py(new — the oracle capture script, shipped for reproducibility), plustest_v4_matrix.py,test_naming_guard.py,test_base_estimator.py,test_v4_inference_policy.py._compute_pscore/_compute_or), the never-treated comparison fork, and the survey-pweight path. All reproduce bit-identically after the relocation. Parity between two callers of a relocated engine cannot detect a transcription slip in the move itself, which is why the absolute pins exist. The no-covariate lanes are deliberately not committed per-method (dr/ipw/reg coincide there, so identical literals would imply coverage they do not provide); that convergence is asserted live instead, and the covariate lanes' mutual distinctness is itself asserted.mypy diff_diffclean at the CI-pinned toolchain;ruffandblack --checkclean.08_triple_diff.ipynb(new staggered-mode section),16_survey_did.ipynband06_power_analysis.ipynball execute end-to-end locally and are committed output-free.Security / privacy