Skip to content

Add workflow improvements to reduce PR review rounds#157

Merged
igerber merged 2 commits intomainfrom
workflow-analysis-improvements
Feb 16, 2026
Merged

Add workflow improvements to reduce PR review rounds#157
igerber merged 2 commits intomainfrom
workflow-analysis-improvements

Conversation

@igerber
Copy link
Copy Markdown
Owner

@igerber igerber commented Feb 16, 2026

Summary

  • Add safe_inference() utility to diff_diff/utils.py for NaN-safe inference computation (t_stat, p_value, CI computed together with NaN gate)
  • Add assert_nan_inference() test helper to tests/conftest.py for validating NaN consistency across all inference fields
  • Add 10 tests for safe_inference() covering NaN/zero/negative/Inf SE, valid SE with normal and t-distribution, return types
  • Add prescriptive inference rules to CLAUDE.md (design pattern Prepare v0.2.0 release #7, test guidelines, checklist items)
  • Enhance pre-merge-check patterns with 4 concrete grep-based checks (inline inference, zero-SE fallback, missing get_params, unguarded CI)
  • Add methodology cross-check to plan review (Registry alignment in Dimension 4)
  • Add known anti-patterns section to Codex review prompt (inline inference P1, missing downstream params P1, partial NaN guard P0)
  • Integrate pre-commit pattern checks into submit-pr and push-pr-update skills
  • Document safe_inference() migration of existing call sites as follow-up in TODO.md

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - safe_inference() is a utility wrapper around existing compute_p_value() and compute_confidence_interval(), no new methodology
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: tests/test_utils.py (10 new tests in TestSafeInference class), tests/conftest.py (new assert_nan_inference helper)
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

Add safe_inference() utility for NaN-safe inference computation,
assert_nan_inference() test helper, enhanced pre-merge pattern checks,
methodology cross-checks in plan review, anti-pattern detection in
Codex review, and pre-commit pattern checks in submit/push skills.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Overall Assessment: ⚠️ Needs changes

Executive Summary

  • Added safe_inference() with NaN gating plus tests; no estimator math/SE defaults changed.
  • Added NaN inference test helper and workflow guidance updates.
  • New workflow grep patterns use PCRE tokens without grep -P, so the checks likely won’t match and can miss inline inference/zero‑SE cases.
  • Tests not run.

Methodology
No findings. Registry reviewed; this PR does not change estimator equations/assumptions or SE definitions, only adds the helper in diff_diff/utils.py:L150-L181.

Code Quality

  1. Severity: P2. Impact: Pattern checks intended to catch inline inference and zero‑SE fallbacks will not match because \s/\b aren’t recognized by default grep, so the checks can silently miss methodology‑critical issues. Concrete fix: Use grep -P (PCRE) with the current regexes or switch to POSIX classes, e.g. grep -En "t_stat[[:space:]]*=[[:space:]]*[^#]*/[[:space:]]*se" and grep -En "if.*(se|SE).*>.*0.*else[[:space:]]+(0\\.0|0)" (and drop \\b). Update in .claude/commands/pre-merge-check.md:L34-L43, .claude/commands/push-pr-update.md:L124-L132, .claude/commands/submit-pr.md:L136-L144, TODO.md:L76-L79.

Performance
No findings.

Maintainability
No findings.

Tech Debt
No findings.

Security
No findings.

Documentation/Tests

  1. Severity: P3. Impact: Changes rely on new tests but they were not run as part of this review, so behavior is unverified. Concrete fix: Run pytest tests/test_utils.py -k safe_inference or the full test suite.

Replace \s with [[:space:]] and drop \b in grep patterns across skill
files and TODO.md. The PCRE tokens silently fail to match in BRE/ERE
mode, defeating the purpose of the pattern checks.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@igerber
Copy link
Copy Markdown
Owner Author

igerber commented Feb 16, 2026

/ai-review

@github-actions
Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 5cde1b7e5d693d1308fc7c3e25db67fde513a549


Overall assessment: ✅ Looks good

Executive Summary

  • Added safe_inference with NaN-gated inference outputs to prevent misleading stats when se is invalid. diff_diff/utils.py:L150-L181
  • Tests cover NaN/zero/inf SEs, distribution choice, and CI behavior for safe_inference. tests/test_utils.py:L527-L621
  • Added assert_nan_inference helper to standardize NaN-consistency checks across inference fields. tests/conftest.py:L141-L175
  • No estimator/SE/identification or default behavior changes in estimator code paths.

Methodology
No findings. Reviewed docs/methodology/REGISTRY.md; no estimator math/SE/identification or default changes in this PR, and safe_inference is a new utility not wired into estimator paths.

Code Quality
No findings.

Performance
No findings.

Maintainability
No findings.

Tech Debt
No findings.

Security
No findings.

Documentation/Tests
No findings.

Tests not run (not requested).

@igerber igerber merged commit 27eceee into main Feb 16, 2026
10 checks passed
@igerber igerber deleted the workflow-analysis-improvements branch February 16, 2026 20:23
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