Surface PowerAnalysis simulation-failure count and narrow except clause

[igerber]

Summary

• Narrow the bare except Exception in simulate_power() to (ValueError, np.linalg.LinAlgError, KeyError, RuntimeError, ZeroDivisionError). Programming errors (TypeError, AttributeError, etc.) now propagate instead of being absorbed into the simulation-failure counter.
• Surface the primary-effect failure count on the result object as SimulationPowerResults.n_simulation_failures, include it in summary(), and add it to to_dict() so it round-trips through to_dataframe().
• Preserve existing behavior: the > 10% proportional UserWarning still fires per effect size, and the all-failed escape still raises RuntimeError.

Methodology references (required if estimator / math changes)

• Method name(s): simulate_power() (simulation-based power analysis).
• Paper / source link(s): No estimator math changed. This PR adjusts exception-handling scope and exposes an internal counter on the result object.
• Any intentional deviations from the source (and why): None. The behavior is now explicitly documented under the PowerAnalysis section of docs/methodology/REGISTRY.md.

Validation

• Tests added/updated (tests/test_power.py::TestSimulatePower):
  • Failure counter is populated on partial-failure runs and is 0 on clean runs.
  • TypeError propagates out of simulate_power (programming-error contract).
  • Proportional UserWarning still fires above 10% failure rate on the same per-effect-size message.
  • n_simulation_failures survives to_dict() serialization.
• Full TestSimulatePower, TestSimulateMDE, TestSimulateSampleSize, TestEstimatorCoverage suites green (64 tests).
• Backtest / simulation / notebook evidence (if applicable): N/A — warning-contract and result-surface change, no numeric-contract change.

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Audit context: resolves finding #11 from the axis-C (silent fallback) slice of the in-flight silent-failures audit. Next up in the plan sequence is PR #7 (axis-A SyntheticDiD diagnostics scale-parity, post-PR #312).

[github-actions]

Overall Assessment

✅ Looks good. I did not find any unmitigated P0/P1 methodology or correctness issues in the diff. Highest remaining issue is P2.

Executive Summary

• simulate_power() is the only affected method, and the diff does not change estimator math, weighting, identification assumptions, or variance/SE formulas.
• The narrowed exception-handling contract and surfaced primary-effect failure count are explicitly documented in the Methodology Registry, so there is no methodology blocker.
• The new failure counter is wired through SimulationPowerResults, summary(), and to_dict() on the primary-effect path.
• SimulationPowerResults now has a new required dataclass field, which is a backward-incompatible public __init__ change for callers that construct result objects directly.
• The new tests cover partial failures and TypeError propagation, but they do not cover the all-failed RuntimeError escape that the PR says is preserved.

Methodology

• P3 Informational. Impact: the only behavioral change is the per-replicate exception filter plus surfacing the primary-effect failure count; there is no estimator-formula, weighting, identification, or SE/variance change in diff_diff/power.py:L2251-L2312. This contract is explicitly documented in docs/methodology/REGISTRY.md:L2356-L2356, so under the review rubric it is not a defect. Concrete fix: none.

Code Quality

No findings.

Performance

No findings.

Maintainability

• P2 Impact: diff_diff/power.py:L1020-L1040 adds n_simulation_failures as a required dataclass field. SimulationPowerResults is a documented public class with a public constructor surface in docs/api/power.rst:L70-L78 and docs/api/_autosummary/diff_diff.SimulationPowerResults.rst:L6-L21, so downstream code that manually instantiates it with the old signature will now fail with TypeError. Concrete fix: keep the new field backward-compatible by moving it after the existing required fields and giving it a default of 0, or making it keyword-only with a default.

Tech Debt

No new untracked technical debt identified in the changed code.

Security

No findings.

Documentation/Tests

• P3 Impact: the new tests in tests/test_power.py:L559-L690 cover partial failures, zero-failure runs, warning emission, and propagated TypeError, but they do not exercise the changed code’s all-failed branch in diff_diff/power.py:L2263-L2274. That leaves the advertised RuntimeError("All simulations failed. Check estimator and data generator.") contract unguarded. Concrete fix: add a test estimator that always raises a caught exception such as ValueError, and assert the exact RuntimeError message.

Verification note: I could not run pytest here because pytest is not installed in this environment.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: a1a522d16723db4a4b3d36fc45cab3bb6f3c8a6a

---

Overall Assessment

✅ Looks good

No unmitigated P0/P1 findings in the diff. Highest remaining item is P3 informational.

Executive Summary

• This re-review resolves the prior non-blocking items: SimulationPowerResults.n_simulation_failures now defaults to 0, avoiding the constructor compatibility issue, and the all-failed RuntimeError path now has direct test coverage in diff_diff/power.py:L1041-L1041 and tests/test_power.py:L629-L645.
• simulate_power() is the only affected method, and the diff does not change estimator math, weighting, identification assumptions, or variance/SE formulas; it only narrows caught exception types and surfaces the primary-effect failure counter in the result object at diff_diff/power.py:L2251-L2312.
• The new exception-handling contract is explicitly documented in the Methodology Registry, so this is a documented behavior change rather than an undocumented methodology deviation at docs/methodology/REGISTRY.md:L2356-L2356.
• The new field is wired consistently through the changed result surfaces: class docs, dataclass field, summary(), and to_dict() at diff_diff/power.py:L997-L1141.
• The added tests cover partial failures, zero-failure runs, propagated TypeError, all-failed RuntimeError, warning-threshold behavior, and serialization of n_simulation_failures at tests/test_power.py:L559-L713.
• I could not execute pytest here because pytest is not installed in this environment.

Methodology

• Severity: P3. Impact: The only behavioral change is the narrowed per-simulation exception filter plus surfacing the primary-effect failure count; this does not alter the simulation power estimator, weighting, identification, or variance/SE computation, and it matches the explicit registry note in diff_diff/power.py:L2251-L2312 and docs/methodology/REGISTRY.md:L2356-L2356. Concrete fix: None.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• No findings. The prior constructor-compatibility concern is resolved by the defaulted dataclass field at diff_diff/power.py:L1041-L1041.

Tech Debt

• No new untracked technical debt identified in the changed files. I did not find a TODO.md blocker relevant to this diff.

Security

• No findings.

Documentation/Tests

• No findings. The prior test gap on the all-failed branch is resolved in tests/test_power.py:L629-L645, and the new tests cover the newly exposed failure counter and programming-error propagation across the changed path in tests/test_power.py:L559-L713.
• Verification note: I could not run the test suite locally because pytest is unavailable in this environment.