Skip to content

Spring cleanup: bootstrap NaN-gating, mypy fixes, doc snippet hardening#219

Merged
igerber merged 1 commit intomainfrom
spring-cleanup
Mar 20, 2026
Merged

Spring cleanup: bootstrap NaN-gating, mypy fixes, doc snippet hardening#219
igerber merged 1 commit intomainfrom
spring-cleanup

Conversation

@igerber
Copy link
Owner

@igerber igerber commented Mar 20, 2026

Summary

  • Migrate imputation_bootstrap.py and two_stage_bootstrap.py to shared compute_effect_bootstrap_stats() from bootstrap_utils.py, gaining NaN filtering, SE<=0 guards, and warning messages that were previously missing
  • Add @overload to solve_ols/_solve_ols_numpy to resolve all 15 mypy tuple-unpacking errors without touching call sites; add assert X is not None guards for Optional indexing across 10 files (81→9 mypy errors, remaining 9 are mixin method attr-defined)
  • Replace blanket except NameError: pass in test_doc_snippets.py with explicit allow-list of 12 context-dependent snippets — unexpected NameErrors now fail the test
  • Update TODO.md: remove resolved bootstrap NaN-gating item, mark doc snippet issue resolved, replace stale "282 pyright errors" with current mypy status

Methodology references (required if estimator / math changes)

  • Method name(s): Multiplier bootstrap inference (Imputation DiD, Two-Stage DiD)
  • Paper / source link(s): Borusyak, Jaravel & Spiess (2024); Gardner (2022)
  • Any intentional deviations from the source (and why): None — migrated to existing shared utility that already implements the correct methodology

Validation

  • Tests added/updated: tests/test_doc_snippets.py (allow-list replaces blanket catch)
  • Full test suite: 1808 passed, 67 skipped, 0 failures
  • mypy: 81→9 errors (all remaining are mixin method attr-defined)
  • Backtest: N/A — no behavioral change to bootstrap inference (shared utility computes identical results when inputs are finite)

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

- Migrate imputation_bootstrap.py and two_stage_bootstrap.py to shared
  compute_effect_bootstrap_stats() for NaN filtering and SE<=0 guards
- Add @overload to solve_ols/_solve_ols_numpy to resolve 15 mypy
  unpacking errors; add assert guards for Optional indexing (81→9 errors)
- Replace blanket NameError catch in test_doc_snippets.py with explicit
  allow-list of 12 context-dependent snippets
- Update TODO.md to reflect resolved tech debt items

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link

Overall Assessment

Looks good. I did not find any unmitigated P0/P1 issues in the diff-scoped estimator/bootstrap changes; the highest finding is P2 in the new doc-snippet test harness.

Executive Summary

Methodology

Code Quality

  • Severity: None. Impact: I did not find a diff-scoped runtime regression in the mypy/typing cleanup or the assertion insertions. Concrete fix: None.

Performance

  • Severity: None. Impact: No diff-scoped performance regression identified. The estimator refactor removes duplicated local bootstrap-stat logic and keeps using the same shared batched weight-generation path. Concrete fix: None.

Maintainability

  • Severity: None. Impact: No maintainability blocker identified in the estimator changes themselves. Concrete fix: None.

Tech Debt

  • Severity: P3. Impact: The refreshed "Large Module Files" table is already inaccurate in the checked-in tree; for example, it lists visualization.py and linalg.py at 1727 lines, but the current files are 1764 and 1829 lines. That makes the new housekeeping data unreliable immediately on merge [TODO.md:24] [diff_diff/visualization.py] [diff_diff/linalg.py]. Concrete fix: regenerate the table from the current tree, ideally via script, before merge instead of hand-editing counts.

Security

  • Severity: None. Impact: No diff-scoped secret-handling, trust-boundary, or code-injection issue identified. Concrete fix: None.

Documentation/Tests

  • Severity: P2. Impact: The new _CONTEXT_DEPENDENT_SNIPPETS allow-list is keyed by positional block{idx} IDs, and those IDs depend on extraction/index order rather than stable snippet identity. Because _extract_snippets() numbers all directive blocks first and shorthand :: blocks second, inserting or converting an earlier snippet can silently move a stale exemption onto a different snippet, weakening the NameError hardening this PR is trying to add [tests/test_doc_snippets.py:69] [tests/test_doc_snippets.py:131] [tests/test_doc_snippets.py:354]. Concrete fix: key exemptions by stable file-relative identity, such as relpath + normalized snippet hash or an explicit inline marker/comment, not by block{idx}.
  • Severity: None. Impact: Relevant regression coverage already exists around the shared helper and both estimator bootstrap paths [tests/test_bootstrap_utils.py:14] [tests/test_imputation.py:897] [tests/test_two_stage.py:999], but I could not execute pytest in this environment because pytest is not installed. Concrete fix: None.

@igerber igerber merged commit c505c19 into main Mar 20, 2026
14 checks passed
@igerber igerber deleted the spring-cleanup branch March 20, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant