Skip to content

CS positional base period + absorbed-FE full-K SE parity vs R#621

Merged
igerber merged 1 commit into
mainfrom
fix/cs-positional-base-absorbed-fe-se
Jul 6, 2026
Merged

CS positional base period + absorbed-FE full-K SE parity vs R#621
igerber merged 1 commit into
mainfrom
fix/cs-positional-base-absorbed-fe-se

Conversation

@igerber

@igerber igerber commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • D3 — CallawaySantAnna positional base period (R did::att_gt parity on gapped grids). Base periods are selected positionally (nearest observed period) via shared _select_base_period / _valid_periods_for_group helpers used across every estimation path (panel fast/vectorized/covariate-reg, repeated cross-sections, universal filters). The pre/post split is corrected to t < g (independent of anticipation; only the post/universal base uses anticipation), verified against a deparse of did 2.5.1 compute.att_gt. Consecutive grids are byte-identical to the old rule; gapped panels (fewer_periods {1,3,4,6}, non_consecutive {1,2,5,7}) now reproduce every R cell instead of NaN-ing cells whose calendar base was unobserved, and base_period="universal" no longer materializes the positional base as a fake zero cell. Aggregated event-study / group SEs on gapped panels are corrected accordingly.
  • D4 — absorbed-FE (absorb=) non-clustered classical/hc1 SE full-K scale (fixest parity). A single scalar vcov rescale (linalg._absorbed_fe_vcov_scale, fail-closed to NaN inference when the full-K residual dof is non-positive) aligns the SE's k with K_full = k_visible + df_adjustment, matching fixest::feols(vcov="iid"/"hetero") and the explicit full-dummy (fixed_effects=) oracle. Previously these SEs sat ~6.5% below fixest even though the reported t-df already used K_full (an internal inconsistency). Applies to TwoWayFixedEffects(vcov_type="classical"), DifferenceInDifferences(absorb=...), MultiPeriodDiD(absorb=...).

Methodology references (required if estimator / math changes)

  • Method name(s): CallawaySantAnna base-period selection (Callaway & Sant'Anna 2021); within-transform (absorb=) finite-sample SE for DifferenceInDifferences / TwoWayFixedEffects / MultiPeriodDiD.
  • Paper / source link(s): R did::att_gt (deparse of did 2.5.1 compute.att_gt); fixest::feols ssc degrees-of-freedom convention. See docs/methodology/REGISTRY.md base-period and within-transform sections.
  • Any intentional deviations from the source (and why): Clustered absorbed-FE CR1 with non-nested FE (e.g. absorb=["unit","time"], cluster="unit", where the time FE are non-nested) still uses k_visible — fixest counts the non-nested FE in the CR1 denominator. This is a small, documented deviation left out of this D4 (non-clustered) scope and tracked in TODO.md; the nested case (unit FE with unit clustering) already matches fixest. The SunAbraham / Wooldridge hc1 deviations documented in REGISTRY are unaffected.

Validation

  • Tests added/updated: tests/test_csdid_ported.py (new TestCSDIDPositionalBasePeriod — helper unit cases incl. an anticipation=2 gapped case base(6)==5, universal, covariate-reg, and repeated-cross-section coverage; tightened the fewer_periods / non_consecutive gapped goldens from abs<0.05/0.5 to 1e-31e-7 with SE assertions), tests/test_estimators_vcov_type.py (new TestAbsorbedFEFullKParity — DiD/MPD/TWFE absorb==full-dummy oracle for classical+hc1, cluster/hc2_bm/SunAbraham-hc1 guards, and a saturated full-K NaN fail-closed case), tests/test_staggered.py, tests/test_methodology_callaway.py.
  • Backtest / simulation / notebook evidence (if applicable): Cross-checked against live R did 2.5.1 (per-cell att+se on the gapped goldens) and fixest (iid 0.202158 / hetero 0.432290 — the absorb path matches the full-dummy oracle and fixest exactly).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

🤖 Generated with Claude Code

https://claude.ai/code/session_0178r8ZK6VRiXbaBiknrWjQB

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

PR Review Report

Overall Assessment: ⚠️ Needs changes

Executive Summary

  • P1: CallawaySantAnna(base_period="universal") now intentionally omits the positional reference cell from group_time_effects, but did::att_gt emits a normalized zero row for that cell. This is not documented as a deviation in REGISTRY.md.
  • CS positional base-period selection itself matches the R source pattern: nearest observed prior period, with anticipation only shifting the universal/post base.
  • Absorbed-FE non-clustered classical/HC1 full-K scaling is methodologically documented and gated to the intended paths.
  • Clustered absorbed-FE non-nested CR1 remains a documented/tracked limitation, so it is not a blocker.
  • I could not run tests in this review environment because numpy is unavailable; review is static.

Methodology

Finding: Undocumented R did::att_gt surface mismatch for universal reference cells

  • Severity: P1
  • Location: diff_diff/staggered.py:L892-L894, tests/test_csdid_ported.py:L1692-L1714, docs/methodology/REGISTRY.md:L640-L664
  • Affected method: CallawaySantAnna, base_period="universal" on gapped grids.
  • Impact: The implementation excludes the positional base/reference period from group_time_effects. R did::att_gt computes the same positional base, but when the current period is the universal base it appends an att = 0 group-time row and advances the result counter. The PR tests now assert omission of (g=6,t=4), while REGISTRY.md says the base-period behavior “matches R did::att_gt() exactly” and only documents the event-study reference row, not the group-time-table deviation. This is a source-material mismatch unless explicitly documented. (rdrr.io)
  • Concrete fix: Add a **Deviation from R:** note in docs/methodology/REGISTRY.md stating that diff-diff intentionally omits universal-mode group-time reference normalization cells from group_time_effects / to_dataframe("group_time"), while retaining the event-study normalization row. Also soften the “matches R exactly” wording to “matches R for base selection and estimable ATT/SE cells.” If exact att_gt table parity is intended instead, materialize the zero reference cells and ensure aggregations/bootstrap skip them as normalization constraints.

Informational: Absorbed-FE full-K variance scaling is documented and scoped

  • Severity: P3
  • Location: diff_diff/linalg.py:L270-L308, diff_diff/linalg.py:L3952-L3962, diff_diff/estimators.py:L2006-L2016, docs/methodology/REGISTRY.md:L342-L361
  • Impact: The scalar rescale applies only to non-clustered classical/hc1 absorbed-FE paths and fail-closes to NaN when full-K residual DOF are non-positive. This is consistent with the registry’s stated fixest full-K convention; fixest documents that non-clustered K.fixef="nonnested" is equivalent to full FE counting. (lrberge.github.io)
  • Concrete fix: None required.

Code Quality

No P0/P1 code-quality findings. The shared _select_base_period() helper reduces drift across panel, covariate-regression, fast, and repeated-cross-section paths.

Performance

No findings. The absorbed-FE variance change is a scalar post-processing step; the CS positional-base lookup is bisect_left over observed periods and should be negligible.

Maintainability

No P0/P1 findings. The one maintainability concern is tied to the methodology finding: the tests now encode an intentional R surface deviation, so the registry should encode it too.

Tech Debt

Informational: Clustered non-nested absorbed-FE CR1 deviation is tracked

  • Severity: P3
  • Location: TODO.md:L113, docs/methodology/REGISTRY.md:L351-L357
  • Impact: The clustered non-nested FE small-sample denominator remains different from fixest, but this is documented in both REGISTRY.md and TODO.md, so it is mitigated under the project’s deferred-work policy.
  • Concrete fix: None required for this PR.

Security

No findings. I saw no changed code paths that introduce secret handling, subprocess execution, or unsafe I/O.

Documentation/Tests

Finding: Test expectation encodes an undocumented deviation

  • Severity: P1
  • Location: tests/test_csdid_ported.py:L1692-L1714
  • Impact: The new test asserts universal reference-cell omission, but the methodology registry does not label that as a deviation from R did::att_gt.
  • Concrete fix: Pair the test with the registry deviation note described above, or change the test/implementation to materialize R-style zero reference cells.

Path to Approval

  1. Add a **Deviation from R:** note to docs/methodology/REGISTRY.md for CallawaySantAnna(base_period="universal") group-time reference cells, explicitly covering group_time_effects and to_dataframe("group_time").
  2. Update the current “matches R exactly” base-period wording to distinguish base selection / estimable cells / aggregations from the intentional group-time reference-row surface difference.
  3. Keep or adjust tests/test_csdid_ported.py:L1692-L1714 so it asserts the documented contract, not an undocumented R mismatch.

@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 29f1538 to 51bde09 Compare July 5, 2026 23:20
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 51bde098db465cc473c48e04112b6dce4907c3c4


PR Review Report

Overall Assessment: ⚠️ Needs changes

Executive Summary

  • Prior P1 is resolved: REGISTRY.md now documents the intentional base_period="universal" group-time reference-cell omission.
  • [Newly identified] P1: universal event-study aggregation on gapped grids still injects a fixed e = -1 - anticipation reference row, but R’s dynamic aggregation places zero reference cells at each cohort’s positional base event time.
  • Absorbed-FE non-clustered classical/hc1 full-K scaling matches the documented fixest convention and is properly gated.
  • No new inline inference / partial NaN-guard anti-patterns were found in the changed files.
  • I could not run tests because this review environment lacks numpy; review is static.

Methodology

Finding: [Newly identified] Universal event-study reference rows are wrong on gapped grids

  • Severity: P1
  • Location: diff_diff/staggered.py:L896-L900, diff_diff/staggered_aggregation.py:L803-L820, diff_diff/staggered_aggregation.py:L950-L961, docs/methodology/REGISTRY.md:L668-L678
  • Affected method: CallawaySantAnna(base_period="universal", aggregate="event_study") on non-consecutive period grids.
  • Impact: The PR now selects the universal base positionally and omits that (g, base) group-time row. But event-study aggregation only sees group_time_effects, then adds a synthetic fixed e = -1 - anticipation row. In R, compute.att_gt materializes the universal base as att = 0 at the actual positional base period, and compute.aggte(type="dynamic") groups by originalt - originalgroup and weights all rows at that event time. On gapped grids, the positional base can be e=-2, e=-3, etc., not e=-1; omitting those zero rows can change negative-horizon dynamic ATT/SEs, and the current registry statement that aggregations are unaffected is not source-compatible. (rdrr.io)
  • Concrete fix: Carry universal reference rows into event-study aggregation at e = base_period_val - g with zero effect and zero IF contribution, or materialize them internally with a reference_period marker while keeping to_dataframe("group_time") omission if desired. Remove the unconditional fixed e=-1-anticipation injection unless that event time is actually a positional reference.

Informational: Prior group-time reference-cell deviation is now documented

  • Severity: P3
  • Location: docs/methodology/REGISTRY.md:L668-L678, diff_diff/staggered.py:L348-L371
  • Impact: The previous review’s P1 is mitigated: the registry now labels the universal group-time reference-cell omission as a Deviation from R.
  • Concrete fix: None.

Informational: Absorbed-FE full-K scaling is methodologically aligned

  • Severity: P3
  • Location: diff_diff/linalg.py:L270-L308, diff_diff/linalg.py:L3941-L3962, diff_diff/estimators.py:L1996-L2016, docs/methodology/REGISTRY.md:L342-L361
  • Impact: The scalar rescale is limited to non-clustered classical/hc1, fail-closes saturated full-K designs to NaN inference, and matches fixest’s rule that nonclustered K.fixef="nonnested" is equivalent to full FE counting. (lrberge.github.io)
  • Concrete fix: None.

Code Quality

No P0/P1 code-quality findings. The shared _select_base_period() helper reduces path drift across panel, covariate-regression, general, and repeated-cross-section estimation paths.

Performance

No findings. The new CS base lookup is bisect over observed periods, and the absorbed-FE variance adjustment is a scalar post-processing step.

Maintainability

Finding: Event-study reference contract is split between estimator filtering and aggregator injection

  • Severity: P2
  • Location: diff_diff/staggered.py:L879-L900, diff_diff/staggered_aggregation.py:L950-L961
  • Impact: Universal reference-cell handling is decided in _valid_periods_for_group(), but event-study normalization is re-created later with a fixed event time. That separation caused the gapped-grid mismatch above.
  • Concrete fix: Centralize universal reference metadata, e.g. return/store {g: positional_base} and let aggregation consume that instead of reconstructing a fixed reference period.

Tech Debt

Informational: Clustered non-nested absorbed-FE CR1 limitation is tracked

  • Severity: P3
  • Location: TODO.md:L113, docs/methodology/REGISTRY.md:L351-L357
  • Impact: The remaining clustered non-nested FE fixest deviation is documented and tracked, so it is mitigated under the project’s deferred-work policy.
  • Concrete fix: None for this PR.

Security

No findings. The diff does not introduce secret handling, subprocess execution, network I/O, or unsafe file operations.

Documentation/Tests

Finding: Missing regression coverage for gapped universal dynamic aggregation

  • Severity: P1
  • Location: tests/test_csdid_ported.py:L1674-L1710, tests/test_csdid_ported.py:L1332-L1378
  • Impact: New tests assert per-cell gapped parity and universal group-time omission, but they do not assert R aggte(type="dynamic") parity for base_period="universal" on a gapped grid. The existing universal dynamic test appears to use a scenario where the fixed e=-1 convention is not challenged.
  • Concrete fix: Add a gapped universal event-study golden where at least one cohort’s positional base has base - g != -1 - anticipation, and assert dynamic egt, ATT, and SE against R.

Path to Approval

  1. Update universal event-study aggregation to include positional reference rows at e = base_period_val - g, with zero effect and zero influence contribution.
  2. Remove or condition the fixed e = -1 - anticipation synthetic row so it does not create an unobserved/wrong reference on gapped grids.
  3. Add a gapped base_period="universal" dynamic aggregation test comparing egt, ATT, and SE to R did::aggte(type="dynamic").
  4. Update REGISTRY.md:L668-L678 so the documented group-time deviation no longer claims event-study/group/simple aggregations are unaffected unless the new test proves that contract.

@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 51bde09 to 9c952f7 Compare July 5, 2026 23:29
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 9c952f7b9182ce93b1c3803b4232cbd76ed9c5c5


PR Review Report

Overall Assessment: ⚠️ Needs changes

Executive Summary

  • Prior P1 is not fully resolved: the code still omits universal positional zero-reference rows from dynamic aggregation, and the new registry/TODO wording incorrectly classifies this as surface-only.
  • The affected method is CallawaySantAnna(base_period="universal", aggregate="event_study") on gapped grids where a cohort’s positional base event time overlaps another cohort’s estimated event time.
  • Absorbed-FE non-clustered classical/hc1 full-K scaling is methodologically aligned with the documented fixest convention and is gated appropriately.
  • No new inline inference or partial NaN-guard anti-patterns were found in the touched estimator paths.
  • I could not run tests: this environment is missing numpy.

Methodology

Finding: Universal dynamic aggregation can still differ from R on gapped grids

  • Severity: P1
  • Location: diff_diff/staggered.py:L896-L898, diff_diff/staggered_aggregation.py:L803-L820, diff_diff/staggered_aggregation.py:L950-L961, docs/methodology/REGISTRY.md:L668-L684, TODO.md:L131
  • Affected method: CallawaySantAnna(base_period="universal", aggregate="event_study")
  • Impact: The PR documents that diff-diff omits R’s universal zero reference cells and instead adds one fixed e = -1 - anticipation row. That documentation says the difference has no numerical impact, but that is not generally true. R’s compute.att_gt materializes the universal base cell with att = 0, and compute.aggte(type="dynamic") groups all rows by originalt - originalgroup and averages them with cohort weights. Therefore a zero reference row at e = base - g affects a dynamic estimate whenever another cohort has an estimable non-reference cell at the same e. Example shape: periods [1, 3, 4, 5, 7], cohorts 5 and 7; cohort 7 has base 5 so its zero reference is e=-2, while cohort 5 has an estimable (g=5,t=3) also at e=-2. diff-diff excludes the (7,5) zero row, so the reported e=-2 ATT/SE is weighted differently than R. This is not mitigated by the current REGISTRY/TODO text because those entries state “surface only” and “all estimable aggregates already match R,” which is the disputed invariant. (rdrr.io)
  • Concrete fix: Either materialize universal positional reference rows internally with zero effect and zero IF at e = base_period_val - g so dynamic aggregation uses the same numerator/denominator/WIF as R, or explicitly document that dynamic ATT/SE can differ from R in overlapping-reference gapped grids and add a test pinning that intentional deviation.

Informational: Absorbed-FE full-K scaling aligns with fixest

  • Severity: P3
  • Location: diff_diff/linalg.py:L270-L308, diff_diff/linalg.py:L3941-L3962, diff_diff/estimators.py:L1996-L2016, docs/methodology/REGISTRY.md:L342-L361
  • Impact: The new scalar rescales only non-clustered classical/hc1 absorbed-FE vcovs, fail-closes non-positive full-K residual df to NaN inference, and leaves clustered/HC2/survey paths alone. fixest documents that K.fixef="nonnested" is equivalent to "full" when SEs are not clustered, while clustered SEs count only non-nested FE. (lrberge.github.io)
  • Concrete fix: None.

Code Quality

No P0/P1 code-quality findings. The shared _select_base_period() / _valid_periods_for_group() helpers reduce drift across panel, covariate-regression, general, and repeated-cross-section paths.

Performance

No findings. The base-period lookup is bisect over sorted observed periods, and the absorbed-FE variance adjustment is a scalar vcov post-processing step.

Maintainability

Finding: Universal reference semantics remain split across estimator filtering and aggregator display

  • Severity: P2
  • Location: diff_diff/staggered.py:L879-L900, diff_diff/staggered_aggregation.py:L950-L961
  • Impact: The estimator excludes the positional base cell, while the aggregator reconstructs a fixed display reference without knowing each cohort’s actual base. This is the root cause of the dynamic aggregation mismatch above.
  • Concrete fix: Store per-cohort universal base metadata and have aggregation consume it directly.

Tech Debt

Informational: Clustered non-nested absorbed-FE CR1 limitation is tracked

  • Severity: P3
  • Location: TODO.md:L113, docs/methodology/REGISTRY.md:L351-L357
  • Impact: The remaining clustered non-nested FE deviation is documented and tracked, so it is not a blocker under the project policy.
  • Concrete fix: None for this PR.

Security

No findings. The touched code does not introduce credential handling, network I/O, subprocess execution, or unsafe file operations.

Documentation/Tests

Finding: Missing regression for overlapping universal reference event times

  • Severity: P2
  • Location: tests/test_csdid_ported.py:L1674-L1756
  • Impact: The new universal gapped tests use a grid where the omitted positional zero reference row does not collide with another cohort’s estimated event time, so they do not catch the dynamic aggregation issue.
  • Concrete fix: Add a gapped universal dynamic test with at least two cohorts where one cohort’s base - g equals another cohort’s estimable t - g; assert either R parity or the explicitly documented alternative behavior.

Path to Approval

  1. Fix dynamic aggregation to include universal zero reference rows at each cohort’s positional e = base - g, with zero IF contribution and correct cohort weight.
  2. Remove or condition the fixed e = -1 - anticipation synthetic row so it does not replace positional reference rows on gapped grids.
  3. Add a gapped universal dynamic regression test for an overlapping-reference case such as periods [1, 3, 4, 5, 7], cohorts 5 and 7.
  4. Update REGISTRY.md, TODO.md, and CHANGELOG.md to remove the “surface only / all estimable aggregates match R” claim unless the implementation is changed to make that true.

@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 9c952f7 to 47593fa Compare July 5, 2026 23:55
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 47593fada39e19717939be2f4a0657ae37cfba37


PR Review Report

Overall Assessment: ⛔ Blocker

Executive Summary

  • Prior analytical P1 for universal positional reference rows is mostly addressed in _aggregate_event_study().
  • A remaining P0 exists: multiplier-bootstrap event-study inference does not materialize those reference rows, so n_bootstrap > 0 can overwrite corrected analytical ATT rows with SE/CI/p-values for a different estimand.
  • balance_e also appears to reintroduce universal reference cells for cohorts filtered out of the balanced analytical sample.
  • Absorbed-FE non-clustered full-K scaling is documented and appropriately gated; the clustered non-nested limitation is tracked.
  • I could not run tests: pytest is unavailable and importing numpy fails.

Methodology

Finding: Bootstrap event-study inference omits universal zero reference cells

  • Severity: P0
  • Location: diff_diff/staggered_bootstrap.py:L223-L307, diff_diff/staggered_bootstrap.py:L686-L800, diff_diff/staggered.py:L2491-L2515, diff_diff/staggered_aggregation.py:L851-L889
  • Affected method: CallawaySantAnna(base_period="universal", aggregate="event_study", n_bootstrap>0) on gapped grids with overlapping reference event times.
  • Impact: The analytical path now correctly adds per-cohort zero reference cells, but the bootstrap helper still builds event-study horizons only from (g,t) pairs present in influence_func_info. In the documented overlap case, analytical e=-2 is the weighted average of a real cell and a zero reference; bootstrap e=-2 is prepared from only the real cell. fit() then keeps the diluted analytical point estimate but overwrites SE/CI/p-value from the undiluted bootstrap estimand. That is silent, wrong statistical output.
  • Concrete fix: Share the universal-reference materialization logic with _prepare_event_study_aggregation(): include zero-reference weights and zero IFs for mixed horizons, omit or all-NaN reference-only horizons so they are not overwritten as se=0, and use the same event-time keys as the analytical path.

Finding: balance_e can reintroduce filtered cohorts through reference cells

  • Severity: P1
  • Location: diff_diff/staggered_aggregation.py:L823-L889
  • Affected method: CallawaySantAnna(base_period="universal", aggregate="event_study", balance_e=...)
  • Impact: The code first filters effects_by_e to cohorts present at balance_e, but then computes cohort_weight from all group_time_effects and appends universal reference cells for all cohorts. That can silently dilute negative horizons with references from cohorts that balance_e removed.
  • Concrete fix: Build reference cells only for the balanced cohort set, or materialize references before applying the balance_e cohort filter and then filter all rows together.

Informational: Absorbed-FE full-K scaling is documented and scoped

  • Severity: P3
  • Location: diff_diff/linalg.py:L270-L308, diff_diff/linalg.py:L3941-L3962, diff_diff/estimators.py:L1996-L2016, docs/methodology/REGISTRY.md:L342-L361, TODO.md:L113
  • Impact: The non-clustered classical/hc1 full-K rescale is aligned with the registry note. The clustered non-nested FE limitation is documented and tracked, so it is not a blocker.
  • Concrete fix: None for this PR.

Code Quality

No additional P0/P1 code-quality findings beyond the duplicated analytical/bootstrap aggregation logic above.

Performance

No findings. The new analytical reference-cell materialization is cohort-linear and should be negligible.

Maintainability

Finding: Event-study aggregation semantics are split across analytical and bootstrap helpers

  • Severity: P2
  • Location: diff_diff/staggered_aggregation.py:L770-L1005, diff_diff/staggered_bootstrap.py:L686-L800
  • Impact: The bootstrap drift is a direct result of two implementations of event-study horizon composition.
  • Concrete fix: Extract a shared helper that returns event-time buckets including finite cells, NaN cells, and universal reference cells, then consume it from both analytical and bootstrap paths.

Tech Debt

No blocker. The group-time display-surface difference for universal zero reference cells is tracked in TODO.md:L131.

Security

No findings. The diff does not introduce credential handling, network I/O, subprocess execution, or unsafe file operations.

Documentation/Tests

Finding: Missing bootstrap regression for the fixed universal-reference case

  • Severity: P2
  • Location: tests/test_csdid_ported.py:L1758-L1812
  • Impact: The new overlap test covers analytical aggregation only. It does not run with n_bootstrap > 0, so the inference overwrite bug is not caught.
  • Concrete fix: Add a deterministic helper-level test for _prepare_event_study_aggregation() showing e=-2 includes the zero reference, plus an end-to-end n_bootstrap>0 regression that verifies the final SE/CI/p-value correspond to the diluted horizon.

Finding: Stale reference-row wording remains

  • Severity: P3
  • Location: diff_diff/staggered.py:L367-L371, docs/methodology/REGISTRY.md:L665-L667
  • Impact: These lines still describe a fixed e=-1-anticipation reference row, while the implementation now uses positional per-cohort reference event times.
  • Concrete fix: Rewrite those sentences to match the new positional reference-cell behavior.

Path to Approval

  1. Update diff_diff/staggered_bootstrap.py::_prepare_event_study_aggregation() to materialize universal positional zero-reference cells exactly like _aggregate_event_study().
  2. Ensure reference-only horizons remain effect=0, se=NaN, p_value=NaN, conf_int=(NaN, NaN) after bootstrap updates.
  3. Make balance_e filter the same cohort set for estimated cells and reference cells.
  4. Add tests for the overlapping-reference gapped panel with n_bootstrap > 0 and for balance_e excluding a cohort whose reference would otherwise be reintroduced.
  5. Clean up the stale fixed-e=-1 documentation wording.

@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 47593fa to 54be4fd Compare July 6, 2026 00:15
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 54be4fd8240690cef53506b7368ad814f44f1e4b


PR Review Report

Overall Assessment: ⛔ Blocker

Executive Summary

  • Prior universal-reference bootstrap drift is mostly addressed: reference cells are now injected before analytical aggregation and bootstrap.
  • P0 remains: universal reference rows can inherit zero/incorrect aggregation weight from an earlier skipped cohort cell, silently corrupting dynamic ATT/SE on unbalanced panels.
  • Absorbed-FE non-clustered full-K scaling is documented in REGISTRY.md and matches fixest’s small-sample correction convention; the clustered non-nested limitation is tracked in TODO.md.
  • No new inline inference / partial NaN-guard anti-patterns found in the changed files.
  • I could not run pytest because numpy is not installed; syntax parsing with ast.parse passed for changed Python files.

Methodology

Finding: Universal reference cells can receive the wrong cohort weight after skipped cells

  • Severity: P0
  • Location: diff_diff/staggered.py:L2394-L2430, consumed by diff_diff/staggered_aggregation.py:L803-L818 and diff_diff/staggered_bootstrap.py:L708-L777
  • Affected method: CallawaySantAnna(base_period="universal", aggregate="event_study"), especially unbalanced/gapped panels and n_bootstrap > 0.
  • Impact: _cohort_meta.setdefault() records the first non-reference row for a cohort, even if that row is a skipped NaN cell with n_treated=0 / no agg_weight. The injected zero reference then carries that bad weight. Dynamic aggregation normalizes on that weight, so an overlapping reference may fail to dilute another cohort’s estimated pre-trend, producing wrong ATT and SE. The registry says universal references are real zero cells weighted into dynamic aggregation/bootstrap, and R did materializes the universal base zero cell before normal estimation. (rdrr.io)
  • Concrete fix: Build reference metadata from fixed cohort mass, not the first row encountered. For panel: derive n_treated/weight from precomputed["unit_cohorts"] or cohort_masks; for survey use fixed cohort survey mass; for RCS use rcs_cohort_masses. If using existing cells, only seed metadata from finite skip_reason is None rows with positive weight.

Informational: Absorbed-FE full-K scaling is documented and scoped

  • Severity: P3
  • Location: diff_diff/linalg.py:L270-L308, diff_diff/linalg.py:L3941-L3962, diff_diff/estimators.py:L1996-L2016, docs/methodology/REGISTRY.md:L342-L361, TODO.md:L113
  • Impact: The non-clustered classical/hc1 rescale is consistent with the registry and with fixest’s K.fixef rule: fixed effects are counted in K, and K.fixef="nonnested" is equivalent to "full" when SEs are not clustered. The clustered non-nested deviation is documented/tracked, so it is not a blocker. (lrberge.github.io)
  • Concrete fix: None for this PR.

Code Quality

No additional P0/P1 code-quality findings beyond the reference-weight bug.

Performance

No findings. The new reference materialization is cohort-linear.

Maintainability

Finding: Event-study weighting semantics remain split across analytical/bootstrap paths

  • Severity: P2
  • Location: diff_diff/staggered_aggregation.py:L770-L1035, diff_diff/staggered_bootstrap.py:L686-L800
  • Impact: The prior bootstrap mismatch and the new reference-weight issue both come from duplicated event-study bucket/weight construction.
  • Concrete fix: Extract one shared helper that returns event-time buckets with finite cells, NaN cells, reference flags, fixed cohort weights, and combined IF inputs.

Tech Debt

No blocker. The clustered absorbed-FE non-nested CR1 limitation is tracked in TODO.md:L113.

Security

No findings. I did not see credential handling, network I/O, subprocess execution, or secrets introduced.

Documentation/Tests

Finding: Missing regression for unbalanced universal reference weights

  • Severity: P2
  • Location: tests/test_csdid_ported.py:L1759-L1816
  • Impact: Current new reference-dilution tests use balanced panels, so they do not catch a skipped first non-reference cell seeding the reference with zero weight.
  • Concrete fix: Add an unbalanced gapped-panel test where a cohort’s earliest non-reference cell is skipped, a later cell is finite, and its universal base reference overlaps another cohort’s event time; assert analytical and n_bootstrap > 0 event-study ATT/SE use the fixed cohort mass.

Path to Approval

  1. Change universal reference metadata construction in diff_diff/staggered.py:L2394-L2430 to use fixed cohort mass from precomputed, not the first group_time_effects row.
  2. Add an unbalanced-panel regression for the skipped-first-cell case, including an overlapping-reference dynamic horizon.
  3. Run the same regression with n_bootstrap > 0 and assert the final event-study SE/CI/p-value correspond to the same weighted estimand as analytical aggregation.

@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 54be4fd to 73a9c53 Compare July 6, 2026 00:28
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 73a9c539d75ef0ccaace257fdf2471819be18f81


PR Review Report

Overall Assessment: ⛔ Blocker

Executive Summary

  • The prior universal-reference skipped-cell issue is improved but not fully resolved.
  • P0 remains: universal reference rows still derive weight metadata from the first estimable cell, not the fixed cohort mass required by the registry/R did aggregation contract.
  • Positional base-period selection is consistent with the local registry and R compute.att_gt’s index-based base selection.
  • Absorbed-FE non-clustered full-K SE scaling is documented in REGISTRY.md; the clustered non-nested deviation is tracked in TODO.md, so it is not a blocker.
  • No new inline inference or partial NaN-guard anti-patterns found in the changed Python files.
  • Could not run tests because numpy is not installed; ast.parse passed for changed Python files.

Methodology

Finding: Universal reference rows still use cell-derived weight metadata

  • Severity: P0
  • Location: diff_diff/staggered.py:L2400-L2443, consumed by diff_diff/staggered_aggregation.py:L803-L819 and diff_diff/staggered_bootstrap.py:L708-L716
  • Impact: The PR now avoids skipped NaN rows, but _cohort_meta is still seeded from the first finite cell for a cohort. On unbalanced panels, that first finite cell can have n_treated below the cohort’s fixed mass because some treated units are missing in that cell’s base/current pair. The injected universal reference then carries the smaller cell count and silently under-weights the zero reference at overlapping event times, corrupting dynamic ATT and SE. The registry says universal reference cells are real zero cells weighted uniformly in event-study and bootstrap (docs/methodology/REGISTRY.md:L668-L682). R did::aggte(type="dynamic") computes cohort probabilities pg from the group column and sampling weights, then normalizes those fixed pg values within each event time; it is not using the first estimable cell’s valid treated count. (rdrr.io)
  • Concrete fix: Build reference metadata from fixed cohort mass, not from group_time_effects: panel unweighted sum(unit_cohorts == g) / cohort_masks[g], panel survey sum(survey_weights[unit_cohorts == g]), and RCS rcs_cohort_masses[g]. Use that same fixed mass for the reference agg_weight consumed by analytical and bootstrap aggregation.

Informational: Positional base-period selection matches the documented contract

  • Severity: P3
  • Impact: _select_base_period() uses sorted-position lookup and the registry now documents that behavior (docs/methodology/REGISTRY.md:L640-L662). This matches R compute.att_gt, which computes the universal/post base via the last index satisfying (tlist + anticipation) < group, and uses index t for varying pre-periods. (rdrr.io)
  • Concrete fix: None.

Informational: Absorbed-FE full-K scaling is documented and scoped

  • Severity: P3
  • Location: diff_diff/linalg.py:L270-L308, diff_diff/linalg.py:L3952-L3962, diff_diff/estimators.py:L2006-L2016, docs/methodology/REGISTRY.md:L342-L361, TODO.md:L113
  • Impact: The non-clustered classical/hc1 rescale follows the documented full-K convention. The remaining clustered non-nested FE difference is explicitly documented/tracked. fixest’s ssc docs state that K.fixef="nonnested" counts non-nested fixed effects and is equivalent to "full" when SEs are not clustered. (lrberge.github.io)
  • Concrete fix: None for this PR.

Code Quality

No P0/P1 code-quality findings beyond the reference-weight correctness bug.

Performance

No findings. The new reference materialization remains cohort-linear.

Maintainability

Finding: Event-study bucket construction remains duplicated, but tracked

  • Severity: P3
  • Location: diff_diff/staggered_aggregation.py:L770-L1032, diff_diff/staggered_bootstrap.py:L686-L800, TODO.md:L131
  • Impact: The analytical and bootstrap event-study paths still duplicate weight/bucket/reference logic. This is now tracked in TODO.md, so it is informational.
  • Concrete fix: None required for approval; future refactor can extract a shared bucket helper.

Tech Debt

No untracked blocker. The clustered absorbed-FE non-nested CR1 limitation is tracked in TODO.md:L113.

Security

No findings. The diff does not introduce credential handling, network I/O, subprocess execution, or secrets.

Documentation/Tests

Finding: Missing partial-mass unbalanced reference regression

  • Severity: P2
  • Location: tests/test_csdid_ported.py:L1835-L1867
  • Impact: The new regression checks that a skipped first cell does not zero the reference weight, but it only asserts n_treated > 0. It does not catch the remaining case where the first finite cell has positive but too-small treated mass.
  • Concrete fix: Add an unbalanced universal event-study test where cohort g has fixed mass 40, its first finite non-reference cell has only 20 valid treated units, and its reference overlaps another cohort’s finite pre-trend. Assert analytical and n_bootstrap > 0 dynamic ATT/SE use the full cohort mass.

Path to Approval

  1. Replace _cohort_meta construction in diff_diff/staggered.py:L2400-L2417 with fixed cohort-mass lookup from precomputed.
  2. Ensure injected reference rows carry that fixed mass through agg_weight for both analytical and bootstrap event-study aggregation.
  3. Add the partial-mass unbalanced-panel regression described above, including a bootstrap case.

@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 73a9c53 to 3b0d8de Compare July 6, 2026 00:43
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 3b0d8de8f6cf3c57a4aa4169d8ed3fff1f15ba0a


PR Review Report

Overall Assessment: ⚠️ Needs changes

Executive Summary

  • The prior universal-reference partial-mass issue is fixed for cohorts with at least one finite non-reference cell.
  • P1 remains/newly identified: universal zero reference cells are still skipped when a cohort has no finite non-reference cell, even though R did materializes the reference before estimating other cells.
  • Positional base-period selection and absorbed-FE non-clustered full-K SE scaling match the updated registry/documented scope.
  • The unbalanced-panel event-study weighting difference and clustered non-nested absorbed-FE CR1 limitation are now documented/tracked, so they are not blockers.
  • Could not run tests: pytest and numpy are not installed in this environment.

Methodology

Finding: [Newly identified] Universal reference omitted for cohorts with no finite real cell

  • Severity: P1
  • Location: diff_diff/staggered.py:L2392-L2429, consumed by diff_diff/staggered_aggregation.py:L803-L819 and diff_diff/staggered_bootstrap.py:L721-L730
  • Impact: The reference injection still requires _cohort_meta from an estimable non-reference cell. If a cohort has a valid universal base but all other cells are non-estimable, for example with control_group="not_yet_treated" and no never-treated controls for the last cohort, Python omits that cohort’s zero reference. R did::compute.att_gt appends the universal zero reference before the later ATT estimation checks, and aggte(type="dynamic") then includes rows at the matching event time with cohort-probability weights. Omitting the reference silently changes dynamic ATT/SE whenever that reference should dilute another cohort’s finite cell. (rdrr.io)
  • Concrete fix: Materialize the universal reference for every cohort with a valid positional base and positive fixed cohort mass, independent of _cohort_meta. Derive n_treated / agg_weight / survey_weight_sum from precomputed fixed cohort mass; use display-only defaults for unavailable n_control; keep the zero IF entry.

Informational: Positional base-period selection matches the documented contract

  • Severity: P3
  • Impact: _select_base_period() implements sorted-position base selection and the t < g pre/post split documented in docs/methodology/REGISTRY.md:L653-L699.
  • Concrete fix: None.

Informational: Absorbed-FE full-K SE scaling is documented and scoped

  • Severity: P3
  • Location: diff_diff/linalg.py:L270-L308, diff_diff/linalg.py:L3941-L3964, diff_diff/estimators.py:L1996-L2016, docs/methodology/REGISTRY.md:L342-L361
  • Impact: The scalar rescale is gated to non-clustered classical/hc1 and fails closed when full-K residual df is non-positive. The clustered non-nested FE difference is documented/tracked.
  • Concrete fix: None.

Code Quality

No additional P0/P1 findings. The new helper centralizes base-period selection across the changed CS paths.

Performance

No findings. Reference materialization remains cohort-linear; the FE rescale is a scalar multiply.

Maintainability

Finding: Event-study bucket construction remains duplicated, but tracked

  • Severity: P3
  • Impact: Analytical and bootstrap event-study bucket/reference logic remains duplicated across staggered_aggregation.py and staggered_bootstrap.py.
  • Concrete fix: No approval action required; tracked in TODO.md:L132.

Tech Debt

No untracked blocking tech debt. The unbalanced-panel CS event-study weighting deviation is documented in docs/methodology/REGISTRY.md:L560-L572 and tracked in TODO.md:L131; the clustered non-nested absorbed-FE CR1 limitation is tracked in TODO.md:L113.

Security

No findings in the changed source files.

Documentation/Tests

Finding: Missing regression for reference-only cohort

  • Severity: P1
  • Impact: Current tests cover skipped first cells and partial reference mass, but not the case where a cohort’s only finite contribution is its universal zero reference.
  • Concrete fix: Add a balanced no-never-treated not_yet_treated test, e.g. cohorts {5,7} on periods {1,3,4,5,7}, base_period="universal", where cohort 7 has no estimable non-reference cells but base (7,5) should be materialized and dilute event time e=-2. Assert analytical and n_bootstrap > 0 paths.

Path to Approval

  1. Remove the _meta is None: continue dependency for universal reference creation in diff_diff/staggered.py.
  2. Build reference metadata from fixed cohort mass in precomputed for panel, survey, and RCS paths.
  3. Add the reference-only cohort regression above, including bootstrap event-study aggregation.

@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 3b0d8de to 8ca08e2 Compare July 6, 2026 00:55
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 8ca08e21be69486a24d458a9d705291cf02a6b68


Overall Assessment: ✅ Looks good

Executive Summary

  • Prior P1 is resolved: universal zero reference cells are now materialized from fixed cohort mass even when a cohort has no finite non-reference cell.
  • CallawaySantAnna positional base-period behavior matches the updated registry contract and is propagated through panel, covariate-regression, vectorized, and RCS paths.
  • Absorbed-FE non-clustered classical/hc1 SE scaling is scoped and documented; clustered non-nested FE remains a documented/tracked deviation, not a blocker.
  • No new inline inference / partial NaN-guard anti-patterns found in the changed paths.
  • Could not run tests in this environment: pytest and numpy are not installed.

Methodology

Finding: Universal reference-only cohort handling is resolved

  • Severity: P3 informational
  • Location: diff_diff/staggered.py:L2392-L2443, diff_diff/staggered_aggregation.py:L851-L931
  • Impact: The previous _cohort_meta dependency is gone. References are created for every cohort with a valid positional base using fixed cohort mass, then included in event-study aggregation with zero IF and NaN inference for reference-only horizons.
  • Concrete fix: None.

Finding: Documented deviations are properly mitigated

  • Severity: P3 informational
  • Location: docs/methodology/REGISTRY.md:L342-L361, docs/methodology/REGISTRY.md:L681-L699, TODO.md:L113, TODO.md:L131-L132
  • Impact: The unbalanced-panel CS event-study weighting difference and clustered non-nested absorbed-FE CR1 difference are explicitly documented/tracked, so they are not approval blockers under the review rules.
  • Concrete fix: None.

Code Quality

No P0/P1 findings. The new _select_base_period() / _valid_periods_for_group() helpers centralize the positional-base contract across the changed CS estimation paths.

Performance

No findings. Reference materialization is cohort-linear; absorbed-FE SE parity is a scalar vcov rescale.

Maintainability

Finding: Event-study bucket logic remains duplicated but tracked

  • Severity: P3 informational
  • Location: diff_diff/staggered_aggregation.py:L803-L931, diff_diff/staggered_bootstrap.py:L686-L800, TODO.md:L132
  • Impact: Analytical and bootstrap event-study/reference bucket construction still duplicate logic, but this is tracked as deferred refactor work.
  • Concrete fix: No approval action required.

Tech Debt

No untracked blocking tech debt found. The new deferred items in TODO.md cover the out-of-scope clustered absorbed-FE CR1 parity and CS unbalanced-panel event-study weighting follow-ups.

Security

No findings in the changed files. No secrets or credential-like material observed.

Documentation/Tests

Finding: Exact reference-only bootstrap regression is still worth adding

  • Severity: P3 informational
  • Location: tests/test_csdid_ported.py:L1870-L1908
  • Impact: The new test covers reference materialization for an all-skipped cohort analytically, and another test covers bootstrap with a partially skipped cohort. A direct n_bootstrap > 0 assertion on the all-skipped/reference-only cohort would better pin the prior P1 end to end, but the source path itself now handles it.
  • Concrete fix: Extend test_universal_reference_materialized_for_all_skipped_cohort to also run with n_bootstrap=199 and assert the overlapping event-time point estimate remains diluted and reference-only inference stays NaN.

Validation note: I attempted targeted pytest execution, but the environment lacks pytest; importing numpy also fails, so no tests were run locally.

@igerber igerber added ready-for-ci Triggers CI test workflows and removed ready-for-ci Triggers CI test workflows labels Jul 6, 2026
@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 8ca08e2 to 5d00187 Compare July 6, 2026 11:09
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 5d00187fbca10c2f43d72822322b30c599392315


Overall Assessment: ✅ Looks good

Executive Summary

  • Prior P1 is resolved: universal reference cells are now materialized per cohort before analytic aggregation and bootstrap, even when a cohort has no finite non-reference cells.
  • CallawaySantAnna positional base-period selection is centralized and propagated through panel fast/vectorized/covariate-regression and repeated-cross-section paths.
  • Absorbed-FE non-clustered classical/hc1 SE scaling matches the documented full-K convention and fail-closes to NaN when full-K residual d.f. is non-positive.
  • Documented deviations in REGISTRY.md / TODO.md are properly mitigated: unbalanced CS event-study weighting and clustered non-nested absorbed-FE CR1 parity.
  • No new P0/P1 methodology, inference, NaN-guard, control-group, or parameter-propagation findings found.
  • Tests could not be run locally because this environment lacks numpy and pytest.

Methodology

No P0/P1 findings.

Finding: Documented deviations are not blockers

  • Severity: P3 informational
  • Impact: The CS unbalanced-panel dynamic weighting deviation and absorbed-FE clustered non-nested CR1 deviation are explicitly documented/tracked, so they are not defects under the review rules.
  • Concrete fix: None.
  • Locations: docs/methodology/REGISTRY.md:L342-L361, docs/methodology/REGISTRY.md:L560-L574, TODO.md:L131-L132

Code Quality

No findings. The new _select_base_period() / _valid_periods_for_group() helpers reduce drift across CS estimation paths.

Performance

No findings requiring action. The bootstrap path remains draw-block chunked and memory-bounded, but the removed fused tiled-GEMM code is a performance rollback, not a methodology defect.

Maintainability

No blocking findings. Event-study bucket construction remains duplicated between analytic and bootstrap paths, but this is tracked in TODO.md.

Tech Debt

No untracked blocking tech debt found.

Security

No findings. No credential-like material observed in the changed files.

Documentation/Tests

Finding: Direct bootstrap regression for all-skipped reference cohort remains useful

  • Severity: P3 informational
  • Impact: The all-skipped cohort reference case is covered analytically, and bootstrap is covered for a partially skipped reference cohort. A direct n_bootstrap > 0 assertion on the all-skipped cohort would better pin the prior issue end to end.
  • Concrete fix: Extend test_universal_reference_materialized_for_all_skipped_cohort to run with n_bootstrap=199 and assert the reference remains present with NaN inference and fixed cohort mass.
  • Location: tests/test_csdid_ported.py:L1870-L1908

Validation note: attempted targeted pytest execution, but /usr/bin/python has neither numpy nor pytest installed.

@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 5d00187 to 40ddd8e Compare July 6, 2026 11:26
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 40ddd8eadbc6ee628966bf88eca1cf94e3ab862f


Overall Assessment: ✅ Looks good

Executive Summary

  • No unmitigated P0/P1 findings found.
  • Prior universal-reference bootstrap concern appears resolved; the all-skipped cohort case now runs both analytical and n_bootstrap=199 paths.
  • CS positional base-period selection and universal reference materialization are documented in REGISTRY.md and propagated through panel/RCS and aggregation/bootstrap paths.
  • Absorbed-FE non-clustered classical/hc1 full-K SE scaling is documented and gated away from clustered, survey, and HC2-family paths.
  • Documented deviations in REGISTRY.md / TODO.md are P3 informational, not blockers.
  • Targeted tests could not be run because this environment lacks numpy and pytest.

Methodology

Finding: Documented CS unbalanced event-study weighting deviation

  • Severity: P3 informational
  • Impact: Unbalanced multi-cell dynamic horizons can differ from R did::aggte because diff-diff uses per-cell weights rather than fixed cohort pg. This is explicitly documented and tracked, so it is not a blocker.
  • Concrete fix: None required for this PR; future fix is tracked in TODO.md.
  • Locations: docs/methodology/REGISTRY.md:L560-L572, TODO.md:L131

Finding: Documented absorbed-FE clustered non-nested CR1 deviation

  • Severity: P3 informational
  • Impact: Clustered absorbed-FE CR1 with non-nested FE remains different from fixest, while the PR intentionally fixes only non-clustered classical/hc1. This is documented/tracked.
  • Concrete fix: None required for this PR; future clustered non-nested full-K adjustment is tracked.
  • Locations: docs/methodology/REGISTRY.md:L342-L361, TODO.md:L113

Code Quality

No findings. I did not find new inline inference anti-patterns or partial NaN guards in the affected paths.

Performance

No blocking findings. The draw-block bootstrap path remains memory-bounded; removal of the fused tiled-GEMM machinery is a performance tradeoff, not a methodology defect.

Maintainability

Finding: Event-study bucket construction duplication is tracked

  • Severity: P3 informational
  • Impact: Analytical and bootstrap event-study bucket construction still duplicate logic, but both consume the same materialized reference cells and the duplication is explicitly tracked.
  • Concrete fix: None required for approval; tracked refactor can extract a shared bucket helper later.
  • Location: TODO.md:L132

Tech Debt

No untracked blocking tech debt found.

Security

No findings. I did not observe credential-like material in the changed files.

Documentation/Tests

Finding: doc-deps.yaml bootstrap chunking note is stale

  • Severity: P3 informational
  • Impact: The note says EfficientDiD/HAD chunking adoption is follow-up, but both still import/use the chunking helpers. This can mislead future drift audits.
  • Concrete fix: Update the note to say draw-block chunking is shared by CallawaySantAnna, EfficientDiD, and HeterogeneousAdoptionDiD.
  • Locations: docs/doc-deps.yaml:L946-L952, diff_diff/efficient_did_bootstrap.py:L105-L188, diff_diff/had.py:L74-L78, diff_diff/had.py:L2181-L2184, diff_diff/had.py:L2231-L2279

Validation note: attempted targeted pytest execution for the new CS positional-base and absorbed-FE tests, but /usr/bin/python has neither numpy nor pytest installed.

…l-K SE parity vs R

Two standard-error parity fixes vs canonical R (SE-audit D3, D4).

D3 - CallawaySantAnna base period is now selected positionally (nearest observed
period) via shared `_select_base_period` / `_valid_periods_for_group` helpers used
across every estimation path (panel fast/vectorized/covariate-reg, repeated
cross-sections, and the universal filters), matching R `did::att_gt` on gapped
(non-consecutive) grids. The pre/post split is corrected to `t < g` (independent of
anticipation; only the post/universal base uses anticipation), verified against a
deparse of `did` 2.5.1 `compute.att_gt`. Consecutive grids are byte-identical to the
old rule; gapped panels now reproduce every R cell (fewer_periods {1,3,4,6},
non_consecutive {1,2,5,7}). For `base_period="universal"`, each cohort's positional
base is materialized as a zero reference cell (att=0, se=NaN, zero influence
function) in group_time_effects / to_dataframe("group_time") at its positional base
event time (`e = base - g`, which can be -2, -3, ... on gapped grids), matching R's
att_gt table AND aggte(type="dynamic") uniformly across the analytical, multiplier-
bootstrap, and balance_e paths -- including the overlapping-reference case where a
cohort's zero base dilutes another cohort's estimated pre-trend at the same event
time (analytical and bootstrap verified vs did 2.5.1 to ~1e-5). The group/simple
aggregations use post-treatment cells only and exclude the reference.

D4 - absorbed-FE (`absorb=`) non-clustered classical/hc1 standard errors now use the
full-K finite-sample scale (`linalg._absorbed_fe_vcov_scale`, a single scalar rescale
of the k_visible vcov; fail-closed to NaN inference when the full-K residual dof is
non-positive), matching `fixest::feols(vcov="iid"/"hetero")` and the explicit
full-dummy oracle. Applies to TwoWayFixedEffects(classical),
DifferenceInDifferences(absorb=), and MultiPeriodDiD(absorb=). Clustered SEs are
unchanged (gated on cluster_ids is None): the k_visible CR1 scale matches fixest for
FE nested in the cluster; non-nested absorbed FE (e.g. time FE with unit clustering)
are a small documented deviation tracked in TODO.md. hc2/hc2_bm (Satterthwaite DOF),
survey vcov, full-dummy fits, and SunAbraham hc1 (auto-clusters) are unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0178r8ZK6VRiXbaBiknrWjQB
@igerber igerber force-pushed the fix/cs-positional-base-absorbed-fe-se branch from 40ddd8e to 9b110d3 Compare July 6, 2026 11:41
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 9b110d315593a33c32cfe72d56c060458807e7f3


Overall Assessment: ✅ Looks good

Executive Summary

  • No unmitigated P0/P1 findings found in the changed diff.
  • CallawaySantAnna positional base-period selection is documented in REGISTRY.md and propagated through panel, covariate-regression, repeated cross-section, aggregation, and bootstrap paths.
  • Universal-base zero reference cells now appear to be materialized before aggregation/bootstrap, with NaN inference for reference-only horizons.
  • Absorbed-FE full-K scaling is limited to the documented non-clustered classical/hc1 paths and skipped for clustered, survey, replicate, and HC2-family paths.
  • Known deviations are documented/tracked in REGISTRY.md / TODO.md, so they are P3 informational only.
  • Static validation only: pytest and numpy are not installed in this environment; git diff --check passed.

Methodology

Finding: Documented CS unbalanced event-study weighting deviation

  • Severity: P3 informational
  • Impact: On unbalanced panels, multi-cell dynamic horizons can differ from R did::aggte because diff-diff uses per-cell aggregation weights rather than fixed cohort pg. This is explicitly documented and tracked, so it is not a blocker.
  • Concrete fix: None required for this PR; future parity work is tracked.
  • Locations: docs/methodology/REGISTRY.md:L560-L572, TODO.md:L132

Finding: Documented absorbed-FE clustered non-nested CR1 deviation

  • Severity: P3 informational
  • Impact: Clustered absorbed-FE CR1 with non-nested FE remains different from fixest; this PR intentionally fixes only non-clustered classical/hc1 full-K scaling.
  • Concrete fix: None required for this PR; clustered non-nested full-K adjustment is tracked.
  • Locations: docs/methodology/REGISTRY.md:L342-L361, TODO.md:L114

Code Quality

No findings. I did not find new inline inference computation or partial NaN-guard anti-patterns in the modified paths.

Performance

No findings. The added base-period helper and scalar absorbed-FE vcov rescale are not performance concerns.

Maintainability

Finding: Event-study bucket construction duplication is tracked

  • Severity: P3 informational
  • Impact: Analytical and bootstrap event-study bucket construction still duplicate logic, but both consume the same materialized reference cells and the duplication is tracked.
  • Concrete fix: None required for approval; tracked refactor can extract a shared bucket helper later.
  • Locations: TODO.md:L133, diff_diff/staggered_aggregation.py:L790-L820, diff_diff/staggered_bootstrap.py:L707-L821

Tech Debt

No untracked blocking tech debt found.

Security

No findings. Secret-pattern scan only hit existing test fixtures (tests/test_openai_review.py), not new credential material.

Documentation/Tests

No PR findings. The new tests cover positional base selection, gapped panel/RCS paths, universal reference materialization, bootstrap all-skipped cohort handling, and absorbed-FE full-dummy parity.

Validation note: targeted tests could not be run here because pytest and numpy are not installed. git diff --check passed.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 6, 2026
@igerber igerber merged commit ced49e8 into main Jul 6, 2026
35 of 36 checks passed
@igerber igerber deleted the fix/cs-positional-base-absorbed-fe-se branch July 6, 2026 13:40
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