Add confidence-interval (ACI) classification for CAT (catIrt-verified) - #257
Conversation
📝 WalkthroughWalkthrough고정 41점 능력 격자에서 ACI 신뢰구간 분류를 수행하는 Rust 코어와 Python API가 추가되었습니다. 분류 결과, 사용 응답 수, 단계별 진단 트레이스를 반환하며 입력 검증과 회귀 테스트가 포함되었습니다. ChangesACI 분류 기능
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PythonCaller
participant fast_mlsirm_ci_classify
participant py_ci_classify
participant core_ci_classify
PythonCaller->>fast_mlsirm_ci_classify: 입력 정규화 및 검증
fast_mlsirm_ci_classify->>py_ci_classify: 분류 파라미터와 응답 전달
py_ci_classify->>core_ci_classify: Rust 코어 호출
core_ci_classify-->>py_ci_classify: decision, n_used, traces 반환
py_ci_classify-->>PythonCaller: Python dict 반환
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Implement mlsirm_core::exposure::ci_classify: single-cut binary-response classification by interim EAP (fixed 41-point [-4,4] grid, standard-normal log prior) with SE = EAP posterior SD; interval theta_hat +/- z_crit * se against theta_cut with STRICT first-crossing decisions and full theta/se/lower/upper counterfactual traces. Verified against R catIrt termCI.R/eapEst.R/catIrt.Rd at commit c9e979e4812c27d95d367a7f097edfe8e93ac8eb (READ). Kingsbury & Weiss (1983), Thompson (2007), and Eggen & Straetmans (2000) NOT method-section verified; cited as historical/background only. Tests: pinned 17-digit independent-Python oracle (all four traces at 1e-12), below/continue paths, full error paths, MC-500 #[ignore] structural invariants (passed). Mutation kills executed: M1 swapped decisions (2 tests fail), M2 point-estimate-vs-cut (2 fail), M3 variance-instead-of-SD (1 fail), M4 n_used off-by-one (2 fail); original restored green. PyO3 py_ci_classify + Python wrapper with validate-before-cast (complex rejection, exact 0/1 responses); pytest TestCiClassify 5 passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Impl-review doc-defect: the PyO3 docstring omitted Eggen & Straetmans (2000) from the NOT-read historical citations required by the approved citation-governance wording (Rust core, Python wrapper, and CHANGELOG already carried it). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
dc6f32e to
d0a3573
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds confidence-interval (ACI) classification as a new CAT stopping-rule path across the Rust core (mlsirm_core::exposure::ci_classify), the PyO3 extension (py_ci_classify), and a thin Python wrapper (fast_mlsirm.ci_classify), with pinned-oracle and validation tests to match the approved reduced-scope contract.
Changes:
- Implement
ci_classifyin Rust, returning decision +n_usedand fulltheta/se/lower/uppertraces. - Expose
ci_classifythrough the PyO3 module and a Python wrapper with pre-cast input validation. - Add Rust + Python tests (pinned oracle, below/continue paths, error paths, and MC invariants) and document in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/exposure_tests.rs | Adds Rust unit tests for ci_classify (oracle + invariants + error paths). |
| tests/test_paper_features.py | Adds Python wrapper tests ensuring the binding/wrapper match pinned outputs and validate inputs. |
| python/fast_mlsirm/exposure.py | Adds the public Python ci_classify wrapper calling _core.py_ci_classify. |
| python/fast_mlsirm/init.py | Re-exports ci_classify from the package top-level. |
| crates/mlsirm-core/src/exposure.rs | Implements the core ci_classify algorithm and result struct. |
| crates/fast-mlsirm-py/src/lib.rs | Adds the py_ci_classify PyO3 binding and registers it in the module. |
| CHANGELOG.md | Documents the new feature, contract summary, and citation governance notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/fast_mlsirm/exposure.py (1)
774-798: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sprt_classify와 거의 동일한 입력 검증 로직 중복.복소수 거부 → ndim 체크 →
c기본값 처리 → responses 0/1 캐스팅으로 이어지는 이 블록은 바로 위sprt_classify(L676-700)와 거의 한 글자씩 동일합니다. 공통 헬퍼(예:_validate_binary_response_inputs(a, b, c, responses))로 추출하면 향후 검증 로직 수정 시 두 곳을 동기화할 필요가 없어집니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/fast_mlsirm/exposure.py` around lines 774 - 798, Extract the duplicated validation flow from sprt_classify and the shown function into a shared helper such as _validate_binary_response_inputs(a, b, c, responses). Move complex-value rejection, 1-D checks, c default handling, and binary response validation/casting into that helper, then have both callers reuse its normalized return values without changing existing validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/fast_mlsirm/exposure.py`:
- Around line 747-771: Update the ci_classify docstring’s References list to add
the formal Nydick (2014) citation for the READ R catIrt sources identified in
the Source status paragraph, matching the existing references in the
corresponding Rust and PyO3 documentation. Keep the historical NOT-read
citations unchanged.
---
Nitpick comments:
In `@python/fast_mlsirm/exposure.py`:
- Around line 774-798: Extract the duplicated validation flow from sprt_classify
and the shown function into a shared helper such as
_validate_binary_response_inputs(a, b, c, responses). Move complex-value
rejection, 1-D checks, c default handling, and binary response
validation/casting into that helper, then have both callers reuse its normalized
return values without changing existing validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19ae477c-efc0-470c-92de-f99cfe75a005
📒 Files selected for processing (7)
CHANGELOG.mdcrates/fast-mlsirm-py/src/lib.rscrates/mlsirm-core/src/exposure.rspython/fast_mlsirm/__init__.pypython/fast_mlsirm/exposure.pytests/test_paper_features.pytests/unit/exposure_tests.rs
Implements the confidence-interval (ACI) classification stopping rule for CAT as
mlsirm_core::exposure::ci_classifywith PyO3 bindingpy_ci_classifyand thin Python wrapperfast_mlsirm.ci_classify.Stacked on #256 (SPRT classification,
seonghobae-sprt).Contract (adversarial spec review: REDUCED-SCOPE approved)
[-4, 4], standard-normal log prior-0.5 theta^2(matches the crateeap_interimconvention).sqrt(E[(theta - theta_hat)^2])), matching catIrteapEst.R.theta_hat +/- z_crit * sevstheta_cut; FIRST STRICT crossing decides (lower > cut-> above,upper < cut-> below; equality continues), matching catIrttermCI.R.n_usedare offline counterfactual replay values (same convention assprt_classify).z_critis caller-supplied; for confidence level L passqnorm((1+L)/2)(catIrtconf.levparameterization).Citation governance
R/termCI.R,R/eapEst.R,man/catIrt.Rdat pinned commitc9e979e4812c27d95d367a7f097edfe8e93ac8eb.Evidence
decision="above",n_used=5with the first-strict-crossing anchorlower_trace[3] <= 0 < lower_trace[4].#[ignore]structural invariants (executed: passed).n_usedoff-by-one -> 2 tests FAILTestCiClassify5 passed (complex-input rejection, exact 0/1 response validation before casts, bool responses, core validation propagation).Adversarial impl-review outcome
Reviewer independently reproduced the pinned oracle (no repo imports; matched at 1e-12), re-ran all Rust/pytest suites, executed adversarial edge cases (single item, cut outside [-4,4], extreme a=1000, huge/tiny z_crit, bad/complex inputs — all finite, correct), and re-executed the M3 drop-sqrt mutation kill (FAIL then restored green).
Status: CLEAN after fix.
Summary by CodeRabbit
새로운 기능
above,below,continue판단과 사용된 응답 수를 제공합니다.문서
버그 수정