feat(v4): EfficientDiD post-fit aggregate() via a lazy recompute kit (2(b) PR-3a, M-023/M-120) - #748
Conversation
…(2(b) PR-3a, M-023/M-120) fit(aggregate=, balance_e=) deprecates (shared NOT_SUPPLIED sentinel, one joint FutureWarning, warn-and-still-work) in favor of post-fit EfficientDiDResults.aggregate(type, balance_e=) on the M-122 mixin contract - a lazy CS-class recompute: fit() computes nothing extra, the results object retains an AggregationKit (per-(g,t) EIF dict referenced canonically + O(n_units) bookkeeping + the post-overall df_survey snapshot), and event_study/group recompute on a throwaway aggregator host while simple relays the stored overall row bit-exact. Inertness pinned at 1e-14 across plain/cluster/survey-TSL/replicate (incl. dropped-replicate and n_valid<=1 degenerate arms) x balance_e x PT-All/PT-Post. Structure: the six aggregation methods move verbatim (one additive per-row df_used capture) to the new diff_diff/efficient_did_aggregation module importable by both estimator and results; efficient_did.py drops below the 2000-line band. Bootstrapped fits fail closed on aggregate() at every level (CS parity; fit-time group rows clear df_used under the bootstrap override; exact replay is a TODO row). The public influence_functions diagnostic deep-copies the kit payload (aliasing would let user mutation corrupt recomputed inference - regression pinned). PT-Post reference provenance ships via a membership-gated reference_period property (SunAbraham rule; absent anchors never synthesized) marking the materialized mechanical zero anchor in the container and correcting plot_event_study's inferred reference on anticipation>0 fits. honest/pretrends admission stays rejected BY DESIGN (no joint ES covariance; both terminal TypeErrors state it). Ledger: M-023 shimmed, new M-120 row claims the reserved balance_e slot (anchor-horizon rule - the CS rule), M-092 scope qualifier, M-093 third amendment, matrix floor 118 + (120,120), naming-guard roster, v4-design phase-5 cell. Docs: REGISTRY M-023/M-120 Note (full retained- buffer enumeration as maxima), llms-full/llms/README/llms-practitioner, api rst + autosummary stub, choosing_estimator, CHANGELOG, TODO + DEFERRED rows, tutorial 15 migrated to post-fit containers and re-executed. Practitioner gains a post-fit aggregation step with a bootstrap carve-out (non-STEPS "aggregation" key). ~120 deprecated call sites swept across six test files with warn hygiene proven under -W error::FutureWarning. Perf (300 units x 10 periods, 15-rep medians): plain fit 23.31->23.35ms and fit-time ES 23.68->23.67ms (noise); plain-fit pickle 9,836->78,262 bytes (the documented kit payload).
Overall assessment⛔ Blocker — one unmitigated P0 finding. Executive summary
MethodologyP0 — [Newly identified] Aggregation mixes mutable public estimates with private fit-time EIFsLocation: Impact: The PR correctly isolates public Concrete fix: Store an immutable/private snapshot of group-time effects and cohort membership in P3 — Documented methodology choicesImpact: The anchor-horizon Concrete fix: None required; all are explicitly documented in Code QualityNo additional findings. The extracted aggregation methods preserve centralized PerformanceP3 — Always-retained EIF payloadImpact: Results and pickles grow by Concrete fix: None required for this PR; the optional MaintainabilityNo additional findings. Tech DebtP3 — Tracked aggregation limitationsImpact: Post-fit bootstrap replay and event-study per-row df provenance remain unavailable. Concrete fix: None required; both are tracked in SecurityNo findings. The retained kit avoids raw unit identifiers, and no secrets were found. Documentation/TestsP2 — Tutorial incorrectly claims covariates are unsupportedLocation: Impact: The summary says “Phase 1 is no-covariates only,” contradicting the implemented and demonstrated doubly robust covariate path. Concrete fix: Replace that takeaway with current covariate-support guidance. Test execution was unavailable because Path to Approval
|
…able public fields (CI review R1)
P0, verified by execution: aggregate('group'/'event_study') read the
MUTABLE public group_time_effects/groups/time_periods while variance
came from the retained fit-time EIFs - mutating a post-treatment public
row shifted the recomputed group ATT by 249 with ZERO se drift (the
mixed plausible-but-invalid state), and popping a cohort from the
public groups list silently dropped rows. The kit now snapshots the
three aggregation inputs at build time (per-row dict copies - values
are scalars/tuples; list copies for groups/time_periods; alpha and
anticipation were already kit fields) and _aggregate_compute reads
exclusively from the snapshots. Regression pin: post-fit aggregation
output invariant under public-field mutation. REGISTRY buffer
enumeration + M-023 ledger notes updated.
P2: tutorial 15's summary takeaway claimed "Phase 1 is no-covariates
only", contradicting the shipped doubly robust covariate path -
replaced with current covariate guidance + the post-fit aggregation
takeaway; notebook re-executed.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment⛔ Blocker — the prior P0 is only partially resolved. Executive summary
MethodologyP0 — Prior finding partially unresolved: carrier still reads mutable public provenanceLocation: Impact: The numerical aggregation uses kit-snapshotted inputs, but This contradicts the registry’s private-snapshot contract at Concrete fix: Store P3 — Documented methodology choicesImpact: Anchor-horizon balancing, bootstrap fail-close, group-df relay, and PT-dependent reference marking are intentional. Concrete fix: None; documented in Code QualityNo additional findings. PerformanceP3 — Always-retained EIF payloadImpact: Results retain Concrete fix: None required; an opt-out is tracked in MaintainabilityNo findings. Tech DebtP3 — Tracked aggregation limitationsImpact: Bootstrap replay and event-study per-row df remain deferred. Concrete fix: None required; tracked in SecurityNo findings or apparent secrets. Documentation/TestsThe previous tutorial covariate error is fixed. Add mutation tests for Runtime tests could not run because NumPy is unavailable. Affected Python files and the notebook parsed successfully. Path to Approval
|
… review R2) Completes the R1 isolation: verified by execution, flipping a PT-All fit's public pt_assumption to "post" made the carrier's reference_period property mark the GENUINE e=-1 estimate (att 0.0945) as a reference row - post_init then zeroed its ATT and NaN'd its inference; a mutated live anticipation shifted which row got zeroed. The kit now snapshots pt_assumption and the n_treated+n_control total, the ES carrier is built with the kit's pt_assumption/anticipation/alpha (never the live public fields), and the simple relay takes alpha and n from the kit. Regression pin extended: post-fit aggregation output and container labeling invariant under pt_assumption/anticipation/alpha mutation, PT-Post fits still mark their genuine anchor. REGISTRY and M-023 ledger snapshot enumerations updated.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0/P1 findings. Executive summary
MethodologyP3 — Documented implementation choicesImpact: Anchor-horizon balancing, bootstrap fail-close, group DF relay, and PT-dependent reference marking intentionally differ from other estimators. Concrete fix: None. Documented in The prior P0 is resolved in Code QualityNo findings. New inference paths consistently use PerformanceP3 — Always-retained EIF payloadImpact: Results retain Concrete fix: None required; an opt-out is tracked in MaintainabilityNo findings. Extracting the shared aggregation mixin reduces estimator-module size without changing formulas. Tech DebtP3 — Tracked limitationsImpact: Bootstrap replay and event-study per-row DF provenance remain unavailable. Concrete fix: None for this PR; both are tracked in SecurityNo findings or apparent secrets. Documentation/TestsNo findings. Mutation isolation, empty surfaces, bootstrap behavior, survey DF cases, and parameter validation have focused coverage in Runtime execution was not possible in the review environment; affected Python files parsed successfully. |
1 participant
Summary
EfficientDiD.fit(aggregate=, balance_e=)(sharedNOT_SUPPLIEDsentinel, one joint FutureWarning, warn-and-still-work) in favor of post-fitEfficientDiDResults.aggregate(type, balance_e=)on the M-122 mixin contract - ledger row M-023 flips to shimmed and the new row M-120 claims the reserved balance_e slot (2(b) PR-3a of the v4 program).fit()computes nothing extra; the results object retains anAggregationKit(the per-(g,t) EIF dict as the canonical payload, O(n_units) bookkeeping, and the post-overalldf_surveysnapshot);aggregate('event_study'/'group', balance_e=)recomputes on a throwaway aggregator host andaggregate('simple')relays the stored overall row bit-exact. Post-fit output matches the fit-time surfaces at 1e-14 across plain/cluster/survey-TSL/replicate fits (incl. dropped-replicate andn_valid<=1degenerate arms), everybalance_e, and bothpt_assumptionregimes.df_usedprovenance capture) into the newdiff_diff/efficient_did_aggregation.py, importable by both the estimator and the results module;efficient_did.pydrops below the 2000-line monitoring band.aggregate()at every level (CS parity); the fit-time bootstrap override clears the analyticaldf_usedprovenance; exact post-fit replay is a tracked TODO row. The publicinfluence_functionsdiagnostic deep-copies the kit payload (an aliased public field would let user mutation silently corrupt recomputed inference - regression-pinned).reference_periodproperty (SunAbraham rule - absent anchors are never synthesized) marks the materialized mechanical zero anchoris_referencein the container and correctsplot_event_study's inferred reference onanticipation>0PT-Post fits.-W error::FutureWarning.Methodology references (required if estimator / math changes)
docs/methodology/papers/chen-santanna-xie-2025-review.md.Note (post-fit aggregate() - rows M-023/M-120)- the lazy-kit memory contract (store_eifnow governs only the public field), the anchor-horizonbalance_erule (the CS rule, divergent from Imputation/TwoStage's balanced-window rule), uniform bootstrap fail-close incl. the relayable 'simple' level (by decision, CS parity), the per-rowdf_usedgroup relay (divergence from CS's scalar broadcast), and the regime-dependent PT-Post reference marking.Validation
tests/test_aggregate_contract.py(TestEfficientShim + TestEfficientAggregate + internal-caller pin, 35 tests incl. the roster floor),tests/test_event_study_consumers.py(real-container rejection pin),tests/test_visualization.py(PT-Post plot-reference pin),tests/test_practitioner.py(handler branch + non-STEPS-key pins),tests/test_guides.py(shim-line pin),tests/test_v4_matrix.py/tests/test_naming_guard.py(ledger pins), plus the sweep acrosstests/test_efficient_did.py,tests/test_methodology_efficient_did.py,tests/test_efficient_did_validation.py,tests/test_survey_phase3.py.docs/tutorials/15_efficient_did.ipynbmigrated and re-executed clean via nbmake; published-parity (HRS/Table-6) and MC suites re-run green post-migration. Perf: plain fit 23.31->23.35 ms and fit-time ES 23.68->23.67 ms (noise); plain-fit pickle 9,836->78,262 bytes (the documented kit payload).Security / privacy