{{ message }}
Wave 3 estimator observability: HonestDiD M=0 test, Wooldridge canonical-link warning, ARP vertex diagnostic#453
Merged
Merged
Conversation
…g mock The test was using `time.time() - t0 < 0.5s` as a proxy for "M=0 took the fast path." Wall-clock proxies on shared CI runners are flaky, so the test was `skipif(CI=="true")` and only ran locally. The fast path is `_compute_worst_case_bias(... M=0) -> 0.0` at honest_did.py:1650, which means `scipy.optimize.linprog` is never reached. Direct correctness signal: `mock.patch` on `diff_diff.honest_did.optimize.linprog` plus `assert_not_called()`. CI-safe, instantaneous, and verifies the actual short-circuit path rather than a timing proxy. Drops the CI skipif decorator and the unused `import os`. Tier A row 1 of post-Wave-2 backlog. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`_enumerate_vertices` was swallowing `np.linalg.LinAlgError` on every
basis with `try / except / continue`, and `_compute_arp_test` returned
False (conservative non-rejection) when the returned list was empty.
Users had no way to tell whether the test "did not reject" because the
data didn't support rejection or because the vertex search was
numerically pathological (singular A_sys on every basis, degenerate
moment-inequality system).
Instrument `_enumerate_vertices` with three counters (`n_total`,
`n_linalg_error`, `n_infeasible`) and emit a `RuntimeWarning` at
function exit when:
1. `vertices == []` after `n_total > 0` bases tried — enumeration
exhausted; caller will fall back to conservative non-rejection.
2. `vertices != []` but `n_linalg_error / n_total >= 0.5` —
enumeration heavily constrained; recovered vertices may be
numerically fragile.
`RuntimeWarning` (not `UserWarning`) marks this as a numerical /
algorithmic signal rather than a user-input issue. `stacklevel=3` so the
warning surfaces at `_compute_arp_test`'s caller, matching the codebase
convention for one-level-deep helper warnings.
No changes to the return type, the caller (`_compute_arp_test`), or
the algorithm semantics — the previous silent-skip behavior is fully
preserved, the diagnostic is purely additive.
Tier-A row in the post-Wave-2 backlog (TODO.md, item 6, PR #334
reference). Adds new `TestARPVertexEnumeration` class in
test_methodology_honest_did.py with three cases: exhausted enumeration,
heavy rejection, healthy enumeration.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Items 1 and 6 from the post-Wave-2 backlog are now shipped: - HonestDiD test_m0_short_circuit (mock-based fix, commit bc0bf39) - HonestDiD ARP vertex-rejection diagnostic (commit 0bfaabba) Removals: - Tier-A bullets for items 1 and 6 - Methodology/Correctness row for HonestDiD ARP vertex-rejection (PR #334) Item 5 (WooldridgeDiD method/outcome pairing) reframed in place — PR #453 R1 review pointed out that the original "canonical link requirement (W2023 Prop 3.1) not enforced" framing misrepresented Wooldridge (2023): Table 1 lists Gaussian/OLS for "any response" and logistic-Bernoulli for "binary OR fractional", so neither OLS-on-binary nor logit-on-fractional is a Prop 3.1 violation. A useful hint still exists at the *efficiency* level (OLS on binary is consistent but logit is typically more efficient for inference), but should not be framed as a methodology violation. Both the Methodology/Correctness row and the Tier-A bullet for item 5 are rewritten to reflect this. Item 5 stays open with the corrected framing for a future wave to address. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
igerber
force-pushed
the
wave-3-observability
branch
from
a6c562c to
7c0bdb2
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good Executive Summary
Methodology No findings. The HonestDiD change does not alter the estimator, identified set, or inference surface. Delta^RM still uses the documented conservative naive FLCI path because ARP remains disabled pending calibration Code Quality No findings. Replacing the timing proxy with a direct mock-based assertion is a better correctness signal for the Performance No findings. The added counters and warning checks are O(1) per enumerated basis and do not change the combinatorial search complexity Maintainability No findings. The TODO cleanup matches the actual state of the branch, including the corrected Wooldridge framing Tech Debt No findings. Removing the resolved HonestDiD TODOs is appropriate, and the remaining HonestDiD ARP calibration debt stays tracked separately Security No findings. Documentation/Tests
|
P3 — `test_enumerate_vertices_warns_on_heavy_rejection` previously used a fixture (X_tilde with 2 unique row types, 5 moments → all 3-row bases singular) where the assertion could pass via the `exhausted` warning instead of the intended `heavily constrained` branch. The branch was effectively untested. Rewrite the fixture: 5 moments × 1 nuisance column, C(5,2)=10 bases. By design, 6 bases trip LinAlgError (pairs among the singular-X_tilde indices) and 4 bases produce feasible vertices (each pairs a positive X_tilde with the unique negative X_tilde at index 4). 60% rejection rate hits the heavily-constrained branch specifically, not exhaustion. Switched to `pytest.warns(RuntimeWarning, match="heavily constrained")` so the test now fails if the message changes or if the wrong branch fires. Added a `len(vertices) >= 1` assertion to guard against the fixture inadvertently producing the exhausted-branch outcome. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Wave 3 of multi-wave tech-debt paydown. Three Tier-A items bundled under a coherent "estimator observability" theme — each commit is independently rollback-able.
bc0bf39aHonestDiDtest_m0_short_circuit: replace wall-clockelapsed < 0.5sproxy withmock.patchonscipy.optimize.linprog+assert_not_called(). CI-safe (no timing dependency), instantaneous, and a direct correctness signal — the M=0 fast path in_compute_worst_case_biasis supposed to skip the LP solver, so verifying the mock was never called is exactly right.cf363048WooldridgeDiD canonical-link warning: module-level_warn_if_canonical_link_violated(method, y, stacklevel)helper called from_fit_ols,_fit_logit,_fit_poissonatstacklevel=3(matching the existing_warn_and_fill_nan_cohortconvention). Four mismatches detected: ols+binary → recommend logit; ols+count → recommend poisson; logit+fractional → note QMLE consistent but not canonical; poisson+binary → recommend logit. Estimator remains consistent under QMLE; warning surfaces the canonical-link violation (W2023 Prop 3.1) that breaks numerical equivalence between QMLE-direct and OLS-by-imputation paths. REGISTRY.md gains a one-bullet note.350ce841HonestDiD ARP vertex-rejection diagnostic:_enumerate_verticesnow tracksn_total/n_linalg_error/n_infeasiblecounters and emits aRuntimeWarningwhen (a) enumeration exhausts without feasible vertices, or (b) ≥ 50% of bases were rejected for LinAlgError. The previous silent-skip behavior is fully preserved (continueon LinAlgError, returnList[np.ndarray]); the warning is purely additive observability. No caller modifications.a6c562c4TODO.md: remove 3 Tier-A bullets + 2 Methodology/Correctness rows (item 1: Tier-A only; items 5 + 6: both surfaces).Net diff: 6 files, +284/-28.
Tests
tests/test_methodology_honest_did.py, including newTestARPVertexEnumeration(3 cases: exhausted enumeration, heavy rejection, healthy enumeration). Oldtest_m0_short_circuitrewritten — drops the@skipif(CI=="true")decorator (CI-safe now).tests/test_wooldridge.py, including newTestCanonicalLinkWarning(6 cases: 3 positive + 3 negative warning paths).Test plan
pytest tests/test_methodology_honest_did.py -v— 30 passpytest tests/test_wooldridge.py -v— 112 passpytest tests/test_methodology_honest_did.py::TestOptimalFLCI::test_m0_short_circuit -v— passes (no skip on CI)pytest tests/test_wooldridge.py::TestCanonicalLinkWarning -v— 6 passpytest tests/test_methodology_honest_did.py::TestARPVertexEnumeration -v— 3 pass🤖 Generated with Claude Code