Conley Wave A: DiD/cluster combo, sparse k-d-tree, callable metric#433
Merged
Conversation
|
Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
Path to Approval
|
igerber
added a commit
that referenced
this pull request
May 14, 2026
…el tests Address CI codex review round 1 (PR #433). P1 #1 — DiD.fit() missing unit-column existence check - The new Conley path validated `unit is not None` but never confirmed the named column actually exists, so a bad column name fell through to a raw `pandas` KeyError at `data[unit]` instead of a clear estimator-level message. Adds the same front-door guard that `MultiPeriodDiD.fit` and `TwoWayFixedEffects.fit` already have: if unit not in data.columns: raise ValueError(f"Unit column '{unit}' not found in data") P1 #2 — Combined-kernel estimator-path coverage - The low-level helper and TWFE-with-cluster were covered, but the DiD and MultiPeriodDiD public estimator paths with explicit `cluster=` were not. Adds four new tests in `TestConleyCluster`: * test_did_combined_kernel_finite_se_and_cluster_name: DiD + Conley + cluster='region' produces finite SE, propagates cluster_name to result + to_dict(), differs from no-cluster baseline (combined kernel zeros out cross-cluster pairs). * test_did_combined_kernel_time_varying_cluster_raises: panel time-invariance contract enforced on the DiD path. * test_mpd_combined_kernel_finite_se_and_cluster_name: same finite-SE + cluster_name + to_dict() invariants on MPD. * test_mpd_combined_kernel_time_varying_cluster_raises: panel time-invariance contract on MPD. - test_did_conley_unknown_unit_column_raises (Code Quality P1 regression): asserts the new ValueError fires before the bad pandas lookup. Targeted regression: tests/test_conley_vcov.py + test_estimators.py + test_methodology_twfe.py: 332 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment Executive Summary
Methodology Code Quality
Performance
Maintainability Tech Debt Security Documentation/Tests
Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
Static review only; I did not execute the test suite in this sandbox. |
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment ✅ Looks good Executive summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
… callable metric validation Four mechanical extensions on top of the Phase 1+2 Conley sandwich (PR #426). All four touch the same surface (conley.py + linalg.py + estimators.py + twfe.py + tests/test_conley_vcov.py). - DiD.fit() accepts `unit=<col>` as a fit-time kwarg (NOT on __init__; unused unless Conley is set; not part of get_params/set_params). - Drops the prior fit-time redirect to MultiPeriodDiD. - Validates unit/lag_cutoff/coords/cutoff_km; rejects survey_design and wild_bootstrap with NotImplementedError. - On a 2-period panel, matches MPD(...).fit(post_periods=[1], reference_period=0) bit-exactly (atol=1e-10). - compute_robust_vcov / LinearRegression / TWFE / DiD now accept cluster_ids alongside vcov_type='conley'; meat applies K_total[i,j] = K_space(d_ij/h) * 1{c_i == c_j}. - Validator enforces cluster-time invariance on the panel block- decomposed path (cluster constant within unit across periods). - Per-slice mask construction (NOT full n×n) preserves memory on panel paths; serial-component mask is trivially all-ones under the invariance contract. - TWFE auto-cluster on Conley path still silently dropped; explicit cluster=<col> opts into combined kernel. DiD has no auto-cluster, so opt-in is fully explicit. DiD-vs-TWFE asymmetry documented. - linalg validator's conley + cluster_ids reject and twfe's explicit-cluster reject both dropped. - _compute_spatial_bartlett_meat_sparse uses scipy.spatial.cKDTree.query_ball_tree to build a CSR sparse kernel matrix instead of materializing the dense n×n distance. - Auto-activates for n > _CONLEY_SPARSE_N_THRESHOLD (5_000) AND metric in {haversine, euclidean} AND kernel == "bartlett". - Bartlett-only gate: bartlett at u=1 returns exactly 0 so the sparse path safely drops at-cutoff pairs; uniform at u=1 is 1 and would require closed-interval query semantics incompatible with haversine chord projection roundoff. - Haversine projects to 3-D unit-sphere Cartesian; chord query radius matches arc-length cutoff with a 1e-12 relative epsilon for projection roundoff; exact great-circle is recomputed only for in-range neighbors. - Private _conley_sparse: Optional[bool] kwarg controls the toggle (None=auto, True=force, False=force dense; True with unsupported config raises). - Bit-identity parity vs dense at atol=1e-10 on synthetic fixtures; R parity at atol=1e-6 preserved on all 3 panel R fixtures with _conley_sparse=True forced. - Renames _CONLEY_DENSE_WARN_N -> _CONLEY_DENSE_OOM_WARN_N (20_000) to disambiguate from the new 5_000 sparse threshold; warning text differentiates sparse-eligible vs dense-fallback paths. - _validate_callable_metric_result checks shape (n,n), finite, non-negative, symmetric within atol=1e-10. - Each failure raises a targeted ValueError naming the violated invariant. Previously, malformed callables produced opaque BLAS errors deep in the pipeline. Tests - tests/test_conley_vcov.py: 36 new tests across TestConleySparse, TestConleySparseRParityForced, TestConleyCluster, TestConleyDistanceMetrics extension. - Existing DiD-rejection / TWFE-explicit-cluster-reject / linalg-conley-cluster-reject tests flipped to behavioral asserts. - test_did_conley_matches_mpd_post_periods_1 locks the DiD-vs-MPD bit-exact agreement on a 2-period panel. - Full regression sweep (test_conley_vcov + test_linalg + test_estimators + test_estimators_vcov_type + test_methodology_twfe + test_linalg_hc2_bm): 511 passed. Docs - docs/methodology/REGISTRY.md: new "Combined spatial + cluster product kernel" subsection with math + cluster-time-invariance contract + two-limit-fixture anchors; new "Performance / scale" subsection on the sparse path; new "Callable conley_metric validation" subsection; updated panel-API restrictions table; DiD-vs-TWFE asymmetry paragraph. - CHANGELOG.md: Unreleased Wave A entry; Phase 1+2 entry's "DiD continues to raise" / "cluster_ids raises" text updated to reflect the lifted rejects. - diff_diff/guides/llms.txt + llms-full.txt: DiD support + combined kernel + sparse path documented; restrictions list refreshed. - README.md: catalog one-line refresh. - TODO.md: rows for the four Wave A items removed; rows 121 (Conley + survey/weights, Bertanha-Imbens 2014) and 122 (SyntheticDiD Conley path, spatial-block bootstrap) retained for later waves. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… leak Address Wave A codex review round 1 findings. P0 #1 — Sparse haversine wrong at cutoff > π·R_earth - `_compute_spatial_bartlett_meat_sparse` computed `chord_radius = 2·sin(cutoff/(2R))` directly. That function is monotone only on [0, πR]; for cutoff > π·R (~20,015 km, half-Earth circumference) it shrinks again, and the kd-tree silently drops pairs that still have positive Bartlett weight. The dense path saturates at πR via `_haversine_km`'s `np.clip(a, 0, 1)`; the sparse path now mirrors that by clamping the arc to π radians before the chord computation: arc_radians = min(cutoff / R_earth, π) chord_radius = 2 · sin(arc_radians / 2) At cutoff >= πR, chord_radius reaches 2 (sphere diameter), so the kd-tree captures all pairs. P0 #2 — DiD + Conley + absorb leaks demeaned time into the helper - `DifferenceInDifferences.fit()` built `_conley_time_arr`, `_conley_unit_arr`, and `_conley_coords_arr` from `working_data`, but `absorb` demeans columns in `working_data` (including `time` when `time` is listed in `absorb`, or whenever `time` appears in `vars_to_demean`). The Conley helper would then partition the within-period spatial sandwich on residualized floats instead of the true pre/post periods. Now reads from the original `data` frame, mirroring `TwoWayFixedEffects.fit` which has the same FWL-composability contract: meat is computed on demeaned scores but the kernel grid uses raw coords + time/unit. P2 — Regression coverage - `test_sparse_haversine_cutoff_above_half_earth_circumference`: forces sparse path with cutoff = 25,000 km (> π·R_earth ≈ 20,015 km); asserts dense/sparse parity at atol=1e-10. - `test_sparse_haversine_cutoff_at_exactly_half_earth_circumference`: boundary test at cutoff = π·R_earth exactly. - `test_did_conley_with_absorb_uses_raw_time_labels`: monkeypatches `_compute_conley_vcov` to capture the `time` arg, asserts the helper sees raw integer 0/1 labels (not absorb-demeaned floats). P3 — Stale docstrings reconciled - `diff_diff/estimators.py` DiD class docstring `vcov_type` entry: "rejected at fit-time" → Wave A support description. - `diff_diff/estimators.py` MultiPeriodDiD class docstring `vcov_type` entry: dropped the "cluster= raises" restriction (Wave A #119 lifts it via the combined kernel). - `diff_diff/linalg.py` `compute_robust_vcov` docstring: `vcov_type` entry updated for the combined spatial + cluster kernel. - `diff_diff/linalg.py` `LinearRegression.__init__` docstring: Conley contract updated for combined cluster kernel + DiD support. Full Wave A regression: 514 passed (up from 511 with the 3 new tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…el tests Address CI codex review round 1 (PR #433). P1 #1 — DiD.fit() missing unit-column existence check - The new Conley path validated `unit is not None` but never confirmed the named column actually exists, so a bad column name fell through to a raw `pandas` KeyError at `data[unit]` instead of a clear estimator-level message. Adds the same front-door guard that `MultiPeriodDiD.fit` and `TwoWayFixedEffects.fit` already have: if unit not in data.columns: raise ValueError(f"Unit column '{unit}' not found in data") P1 #2 — Combined-kernel estimator-path coverage - The low-level helper and TWFE-with-cluster were covered, but the DiD and MultiPeriodDiD public estimator paths with explicit `cluster=` were not. Adds four new tests in `TestConleyCluster`: * test_did_combined_kernel_finite_se_and_cluster_name: DiD + Conley + cluster='region' produces finite SE, propagates cluster_name to result + to_dict(), differs from no-cluster baseline (combined kernel zeros out cross-cluster pairs). * test_did_combined_kernel_time_varying_cluster_raises: panel time-invariance contract enforced on the DiD path. * test_mpd_combined_kernel_finite_se_and_cluster_name: same finite-SE + cluster_name + to_dict() invariants on MPD. * test_mpd_combined_kernel_time_varying_cluster_raises: panel time-invariance contract on MPD. - test_did_conley_unknown_unit_column_raises (Code Quality P1 regression): asserts the new ValueError fires before the bad pandas lookup. Targeted regression: tests/test_conley_vcov.py + test_estimators.py + test_methodology_twfe.py: 332 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P1 [new in R2] — DiD.fit() didn't validate conley_coords shape or
column existence; malformed tuples or missing column names fell through
to opaque IndexError/KeyError downstream. Mirrors the unit-column guard
from R1.
Adds two checks in the DiD Conley block:
- conley_coords must be a 2-element tuple/list of strings (raises if
arity wrong or any element non-string).
- Each named column must exist in `data` (raises ValueError naming
the missing column).
Adds two regression tests in TestConleyEstimatorIntegration:
- test_did_conley_unknown_coord_column_raises
- test_did_conley_malformed_coord_tuple_raises (covers 1-element
tuple and non-string element)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When an explicit cluster=<col> is combined with vcov_type='conley',
the result summary now distinguishes the combined spatial + cluster
product kernel from bare Conley and names the cluster column:
Bare Conley:
"Conley spatial HAC (1999), lag_cutoff=1"
Combined kernel (Wave A #119):
"Conley spatial HAC (1999) + cluster product kernel at region, lag_cutoff=1"
Previously `_format_vcov_label()` ignored `cluster_name` on the Conley
path, so combined-kernel fits printed the same label as pure Conley.
Adds a regression test
`test_twfe_conley_with_cluster_summary_label_names_kernel_and_cluster`
covering the new label format. The existing bare-Conley summary test
now also asserts the absence of the "+ cluster product kernel" suffix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P1 [new in R4] — `_validate_callable_metric_result` accepted any
symmetric / finite / non-negative matrix, even when d(i, i) > 0. The
Conley sandwich relies on K(0) = 1 to reduce the i=j term to the HC0
diagonal X_i ε_i² X_i'; a positive self-distance silently attenuates
the HC0 contribution by K(d_ii / h) < 1 and misstates Conley SEs.
Built-in metrics ("haversine", "euclidean") satisfy d(i, i) = 0 by
construction, so the existing parity tests didn't catch this gap on
the callable surface.
Adds a sixth check to the callable validator:
|d(i, i)| <= 1e-10 for all i, else ValueError naming the violated
invariant ("nonzero self-distance ... requires d(i, i) = 0 so the
kernel reduces to K(0) = 1 on the HC0 diagonal contribution").
Tests:
- test_callable_metric_nonzero_diagonal_raises: symmetric / finite /
non-negative with d(i, i) = 0.5 raises.
- test_callable_metric_near_zero_diagonal_accepted: sub-tolerance
diagonal noise (1e-13) accepted, mirroring the symmetry contract.
- test_pairwise_distance_callable updated: the constant_metric
fixture now zeroes its diagonal (otherwise it would fail the new
check); behaviorally equivalent off-diagonal coverage.
Docs:
- docs/methodology/REGISTRY.md § "Callable conley_metric validation"
extended: 6th check added to the list + a paragraph explaining the
HC0-reduction rationale.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P0 Methodology [new in R5] — `MultiPeriodDiD.fit` built `_conley_coords_arr` / `_conley_time_arr` / `_conley_unit_arr` from `working_data`, which is absorb-demeaned. If a caller listed any of those columns (coords, time, or unit) in `absorb`, the Conley helper would silently partition the within-period spatial sandwich on residualized values instead of the true geography/period labels and misstate Conley SEs. This is the same raw-data contract the local R1 fix established for DifferenceInDifferences (`estimators.py:L538-L561`); MultiPeriodDiD shared the same pattern but wasn't updated in that pass — codex CI R5 caught the holistic-fix omission. Now reads from `data` in MPD, mirroring DiD + TWFE (`twfe.py:L370-L371`). FWL composability holds: the meat is computed on demeaned scores but the kernel grid uses raw coords + time/unit. Adds `test_mpd_conley_with_absorb_uses_raw_coords_and_time` regression: monkeypatches `_compute_conley_vcov` to capture the time + coords args, asserts the helper sees raw integer time labels (0..T-1, not absorb- demeaned floats) AND n_units distinct (lat, lon) pairs (not collapsed to one demeaned mean per unit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… P3 uniform-kernel limit fix
Three findings on R6, addressed via pattern-level fixes rather than
single-site patches, after 5 cross-round findings across 2 mirror
classes (validation-gap, raw-data-vs-working-data) signaled the symptoms
were drifting between estimator surfaces.
P1 (Code Quality) — shared estimator validator
Validation gaps mirrored across DiD (R1, R2) and MPD/TWFE (R6) under
the same `vcov_type='conley'` entry-point contract: missing/unknown
unit column, malformed conley_coords arity, missing coord column,
survey_design rejection, wild_bootstrap rejection. Replaces three
separate inline blocks (one per estimator) with a single
`_validate_conley_estimator_inputs(...)` helper in `conley.py`,
called from `DifferenceInDifferences.fit()`,
`MultiPeriodDiD.fit()`, and `TwoWayFixedEffects.fit()`. Single
source of truth for the seven-check union; future estimator
surfaces (HAD-Conley, SDID-Conley) pick up the same contract by
one-line opt-in.
P2 (Performance) — sparse density gate
The sparse k-d-tree path's CSR storage (~12 bytes/nnz: data +
indices + indptr) loses its memory advantage over dense float64
(8 bytes/cell) at ~67% density; at high density "sparse" can
silently use MORE memory than dense. Adds
`_CONLEY_SPARSE_DENSITY_THRESHOLD = 0.3` (well below break-even
for a comfortable safety margin). After kd-tree build,
`cKDTree.count_neighbors` (shares traversal with the subsequent
`query_ball_tree`) measures actual neighbor density; if it exceeds
the threshold, `_compute_spatial_bartlett_meat_sparse` returns
None, the dispatcher falls back to the dense path, and a
UserWarning surfaces the reason. Locks the auto-sparse contract:
it only fires when it actually saves memory.
P3 (Documentation/Tests) — uniform kernel for huge-cutoff limit
`test_combined_kernel_reduces_to_pure_cluster_at_huge_cutoff` used
Bartlett at cutoff=1e9; Bartlett gives K=1-d/h<1 for d>0, so the
reduction to pure CR1 is asymptotic, not exact. Switched to
uniform kernel (K=1 exactly for all in-cutoff pairs). REGISTRY
note clarifies the kernel choice rationale.
Tests:
- test_sparse_density_gate_falls_back_to_dense_and_warns:
tight-cluster + huge cutoff → 100% density → fallback +
warning + result equals dense.
- test_sparse_density_gate_does_not_trigger_below_threshold:
realistic cutoff → no fallback / no warning.
Full regression sweep: 493 passed (no regressions across
conley_vcov, linalg, estimators, estimators_vcov_type,
methodology_twfe).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ence P1 (Code Quality) [new in R7] — `cluster=<col>` is load-bearing on the Conley combined-kernel paths (Wave A #119) but none of DiD / MPD / TWFE validated that the named cluster column exists in `data` before the downstream `data[self.cluster]` access. A typo like `cluster="missing_region"` fell through to a raw pandas KeyError instead of the estimator-level ValueError pattern the rest of the Conley validation surface now uses. Same class as R1's unit-column guard and R2/R6's conley_coords guard: extends the shared `_validate_conley_estimator_inputs` helper added in R6 with an 8th check `if cluster is not None and cluster not in data.columns: raise ValueError("Cluster column ... not found in data")`. The three call sites in DiD/MPD/TWFE now pass `cluster=self.cluster` through and pick up the new guard via one-line opt-in. Future Conley surfaces that add cluster support get the validator's behavior for free. Tests: regressions on all three estimator surfaces (DiD/MPD/TWFE) asserting `cluster="missing_region"` raises the estimator-level ValueError before any pandas-level error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gressions
Two related P1s on R8 (both about the sparse+cluster interaction).
P1 (Performance) — density gate is now cluster-aware
The R6 density gate counted ALL spatial in-range pairs before the
cluster mask was applied, but the actual CSR matrix stores only
within-cluster pairs (the inner row-loop drops cross-cluster
neighbors before populating row/col/data lists). On large clustered
panels — exactly the use case sparse+cluster is supposed to win —
spatial density could be ~100% while within-cluster density is tiny,
and we'd spuriously fall back to dense O(n²) work.
Fix: when the unrefined spatial density exceeds the threshold AND
`cluster_codes is not None`, refine by counting only within-cluster
in-range pairs (per-cluster kd-trees, summed). The refinement runs
only when needed (the gate is already cleared if spatial density is
below threshold), so the per-cluster sub-tree builds are amortized.
Stores the projected coordinates as `tree_coords` once on the
haversine branch (was `xyz` locally before; renaming makes the
per-cluster build symmetric across metrics).
P1 (Documentation/Tests) — sparse+cluster regression coverage
Sparse coverage previously exercised unclustered dense-vs-sparse
parity; cluster coverage previously exercised dense/estimator
behavior — but the combined sparse+cluster path was load-bearing
and untested. Adds three regressions:
- test_sparse_with_cluster_matches_dense: cross-sectional 600-row
panel, 8 clusters, dense vs forced-sparse parity at atol=1e-10.
- test_sparse_with_cluster_panel_matches_dense: 3-period block-
decomposed panel, 200 units, 5 time-invariant clusters, dense
vs forced-sparse parity at atol=1e-10.
- test_sparse_density_gate_cluster_aware: tight spatial cluster
(100% global density) + 50 small clusters → within-cluster
density << 30%. Locks the refinement contract: NO density
warning, sparse path executes, result matches dense.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P3 (Documentation/Tests, cosmetic) — pure docs polish.
(1) docs/methodology/REGISTRY.md L3083-3092: huge-cutoff CR1 limit
expression had an erroneous trailing prime on X_g, producing a
dimensionally-inconsistent form. The CR1 meat for cluster g is
`(X_g' ε_g)(X_g' ε_g)' = X_g' ε_g ε_g' X_g` (the last X_g is
unprimed; the result is (k, k)). Fix: drop the trailing prime.
(2) CHANGELOG.md Wave A entry: mirror two REGISTRY clarifications that
were already in the methodology document but missing from the
release note prose:
- "Huge-cutoff reduction is EXACT only for `conley_kernel="uniform"`;
for `bartlett` the identity is asymptotic since K_bartlett(u) < 1
for u > 0. The fixture anchor uses uniform for an exact identity
check."
- "Callable conley_metric validation now requires zero diagonal
(`|d(i, i)| ≤ 1e-10`); rationale is the K(0) = 1 reduction to
the HC0 diagonal X_i ε_i² X_i'."
Implementation and tests are correct; this is documentation-only
alignment with the codex R9 P3 finding. Loop stops here — R9 was
already green per the stop criterion, but the user opted to polish.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two P3 doc-only items from R10 (final ✅ Looks good review).
(1) diff_diff/guides/llms-full.txt: the new DiD-Conley example used
DifferenceInDifferences(...) but the import block at the top of
the code sample didn't include it (only LinearRegression,
MultiPeriodDiD, TwoWayFixedEffects). Example wasn't runnable as
written. Added DifferenceInDifferences to the imports.
(2) Doc-contract drift across three summary surfaces — the main
methodology subsections already documented these contracts
correctly, but downstream summary bullets/release notes were stale:
- llms-full.txt:L2031 callable-validation bullet now spells out
"shape (n, n), finite, non-negative, symmetric to atol=1e-10,
AND zero on the diagonal" (was missing the zero-diagonal rule).
- REGISTRY.md edge-case bullet for callable now names "non-zero-
diagonal matrix" alongside the other rejection conditions, with
pointer back to the main subsection.
- CHANGELOG.md Wave A item (2) (combined kernel) now explicitly
lists MultiPeriodDiD(vcov_type="conley", cluster="<col>") along-
side DiD/TWFE; the MPD path is implemented + tested but was
previously omitted from the surface enumeration.
Implementation and tests unchanged; pure docs alignment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
igerber
force-pushed
the
spillover-conley-wave-a
branch
from
cd08440 to
3b05cd2
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good Executive Summary
Methodology No findings. The documented Wave A extensions in Code Quality
Performance No findings. Maintainability No findings. The new shared Tech Debt No findings. Remaining deferred Conley work is still explicitly tracked in Security No findings. Documentation/Tests
|
…ey path + harmonize doc restriction bullets Two P3 doc-and-UX items from R11 (✅ Looks good overall). P3 #1 (Code Quality) — contradictory warning + error on MPD+Conley +wild_bootstrap `MultiPeriodDiD.fit()` had an unconditional UserWarning at the top of fit() saying wild_bootstrap "is not yet supported, using analytical instead" — but the Conley validator below raises NotImplementedError on `vcov_type="conley", inference="wild_bootstrap"`. Users hitting that combination got "warn we're falling back" followed immediately by "actually we're raising," which is contradictory guidance on the same call. Fix: gate the warning on `self.vcov_type != "conley"`. The Conley validator's raise takes precedence. Adds regression `test_mpd_conley_wild_bootstrap_raises_without_warning` asserting the NotImplementedError fires with no analytical-fallback warning. P3 #2 (Documentation/Tests) — restriction bullets understated MPD's reject surface - `llms-full.txt:L2026` only named DiD/TWFE for the `inference="wild_bootstrap"` reject; MPD also rejects this combo via the shared validator. Updated to enumerate all three. - `llms-full.txt:L2027` only named DiD for the estimator-level `survey_design=` reject; MPD and TWFE both raise the same on this combo. Updated to enumerate all three. - `docs/methodology/REGISTRY.md:L3188` had the same drift; updated in parallel and added a parenthetical noting the MPD warning suppression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
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
DifferenceInDifferences(vcov_type="conley").fit(..., unit="<col>")lifts the prior fit-time redirect.unitis a fit-time kwarg (not on__init__; unused unless Conley is set; not part ofget_params/set_params). On a 2-period panel, ATT + SE matchMultiPeriodDiD(...).fit(post_periods=[1], reference_period=0)bit-exactly.compute_robust_vcov/LinearRegression/ TWFE / DiD now acceptcluster_ids/cluster=alongsidevcov_type="conley". Meat appliesK_total[i,j] = K_space(d_ij/h) · 1{c_i = c_j}. On the panel block-decomposed path the validator enforces cluster-time invariance within each unit (raisesValueErrorotherwise). TWFE's default auto-cluster on the Conley path is still silently dropped; explicitcluster=<col>opts into the combined kernel.n > 5_000AND metric ∈{haversine, euclidean}AND kernel =bartlett. Builds a CSR sparse kernel matrix viascipy.spatial.cKDTree.query_ball_tree; haversine projects to a 3-D unit-sphere chord representation with the arc clamped at π (so cutoffs above half-Earth circumference still capture all geometrically eligible pairs). Bit-identity parity vs dense atatol=1e-10; R parity atatol=1e-6preserved with the sparse path force-enabled on the 3 panel R fixtures. Constants:_CONLEY_SPARSE_N_THRESHOLD = 5_000;_CONLEY_DENSE_WARN_Nrenamed to_CONLEY_DENSE_OOM_WARN_N = 20_000to disambiguate from the new sparse threshold.conley_metricvalidation (Fix worktree-rm: detect squash-merged branches via GitHub PR status #123):_validate_callable_metric_resultenforces shape(n, n), finite, non-negative, symmetric toatol=1e-10. Each failure raises a targetedValueErrornaming the violated invariant — replaces the prior opaque BLAS errors for malformed callables.Methodology references (required if estimator / math changes)
conleyreg(Düsterhöft 2021, CRAN v0.1.9, https://github.com/cdueben/conleyreg) — parity benchmark. Stataacreg(Colella, Lalive, Sakalli, Thoenig 2019, IZA DP 12584) — combined-kernel parallel canonical implementation (not parity-tested).conleyregdoes not support this combination. Validated by two limit fixtures (all-unique-clusters reduces to HC0; huge-cutoff reduces to pure within-cluster CR1) rather than R parity. (b) Sparse haversine arc clamped at π — matches dense path'snp.clip(a, 0, 1)saturation; preserves correctness for cutoffs above half-Earth circumference. (c) Panel time-invariance contract oncluster_ids— required so the within-unit serial sandwich's cluster mask is trivially all-ones; cross-sectional path has no such constraint. All documented as**Note:**/**Deviation from R:**labels indocs/methodology/REGISTRY.md§ ConleySpatialHAC.Validation
tests/test_conley_vcov.pyextended by ~1100 lines coveringTestConleySparse(sparse k-d-tree + arc-clamp edge cases),TestConleySparseRParityForced(R parity with sparse forced),TestConleyCluster(combined kernel + time-invariance contract),TestConleyDistanceMetrics(callable validation), plus DiD-with-Conley integration tests (raw-time-label propagation underabsorb, MPD parity atatol=1e-10).conleyregparity atatol=1e-6preserved on the 6 existing benchmark fixtures (3 cross-sectional + 3 panel) under both dense and forced-sparse paths. Bit-identity dense ↔ sparse parity atatol=1e-10on synthetic euclidean + haversine fixtures including the boundary casecutoff = π·R_earth.tests/test_conley_vcov.py,tests/test_linalg.py,tests/test_estimators.py,tests/test_estimators_vcov_type.py,tests/test_methodology_twfe.py,tests/test_linalg_hc2_bm.py.Security / privacy
Generated with Claude Code