Add Kingsbury-Zara (1989) CCAT content-balanced item selection - #254
Conversation
c3892dd to
af8f0ae
Compare
af8f0ae to
7c8fd15
Compare
Rust core ccat_select in mlsirm_core::exposure: zero-coverage group priority, then max target-minus-empirical-proportion discrepancy among eligible groups; within-group max logistic 3PL Fisher information a^2(Q/P)((P-c)/(1-c))^2; lowest-index ties (documented deviation from catR's random tie-break). Primary source NOT read (paywalled, doi:10.1207/s15324818ame0204_6); rule reproduced from R catR nextItem.R cbControl (READ), info formula verified against catR Ii.R/Pi.R. Tests: pinned exact-arithmetic oracles (1e-12), balancing-overrides- global-max-info discriminator, zero-coverage priority discriminator, exhausted-group skip, error paths, MC-500 structural invariants (#[ignore]); 4 executed mutation kills (gap argmax->argmin, info argmax->argmin, zero-priority dropped, guessing factor dropped). PyO3 binding py_ccat_select + Python wrapper with pre-cast group validation (rejects negatives/non-integers/complex) + pytest. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Impl-review MAJOR: with c = 0 and extreme finite theta0 the logistic underflows to P = 0, making the naive q/p * r^2 info form inf * 0 = NaN, which silently corrupted the within-group argmax (x > NaN is false, so a NaN-info item stayed selected). The true limit is 0 (c = 0 gives I = a^2 q p -> 0 as p -> 0); return the limiting value directly. Regression test reads crate outputs; guard-revert mutant executed and killed (1 failed), restore verified green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Impl-review round 2 (3 MAJOR): the naive q/p * r^2 form (and the interim p == 0 guard) failed on extreme-but-valid inputs: (1) subnormal c > 0 with underflowed L gave inf * 0 = NaN; (2) a = 1e154 overflowed to spurious +inf via multiplication order and won the argmax; (3) the p == 0 guard masked a genuinely informative extreme item (a = 1e162, true I = 2.822 > 2.25). Fix: with z = a(theta0 - b), L = sigmoid(z), r = (P - c)/(1 - c) = L exactly, so I = a^2 (1-c)(1-L) L^2 / (c + (1-c)L) (c = 0: a^2 L (1-L)) computed via ln L = -softplus(-z), ln(1-L) = -softplus(z). Regression test pins 80-digit Decimal reference values (1e-9); two executed mutation kills (c==0 branch dropped: 2 failed; naive softplus: 1 failed), restore green; pinned 1e-12 oracles unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
4c81ea9 to
efe1acf
Compare
There was a problem hiding this comment.
Pull request overview
Adds Kingsbury–Zara (1989) constrained CAT (CCAT) content-balanced item selection to the exposure/CAT utilities, implemented in the Rust core and surfaced through the PyO3 binding and Python API.
Changes:
- Implement
mlsirm_core::exposure::ccat_selectwith content-group discrepancy selection + within-group 3PL Fisher-information maximization (numerically stabilized via log-space computation). - Expose the function to Python via
py_ccat_selectand afast_mlsirm.ccat_selectwrapper/export. - Add Rust + Python tests (pinned oracles, edge/regression cases) and document the feature 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 CCAT selection (oracles, invariants, error paths, numeric regressions). |
| tests/test_paper_features.py | Adds Python tests validating the CCAT binding behavior and wrapper-level group validation. |
| python/fast_mlsirm/exposure.py | Adds ccat_select Python wrapper + docstring describing the CCAT rule and citation governance. |
| python/fast_mlsirm/init.py | Re-exports ccat_select and adds it to __all__. |
| crates/mlsirm-core/src/exposure.rs | Implements ccat_select and CcatSelectResult in the Rust core with robust info computation. |
| crates/fast-mlsirm-py/src/lib.rs | Adds PyO3 binding py_ccat_select wiring to the Rust core. |
| CHANGELOG.md | Documents the newly added CCAT selection feature and its provenance/constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughRust 코어에 CCAT 콘텐츠 밸런싱 선택 로직을 추가하고, PyO3 및 Python 공개 API로 노출했습니다. 그룹 discrepancy, 3PL Fisher 정보, 입력 검증, 타이브레이크와 극단 입력 안정성에 대한 테스트도 추가했습니다. ChangesCCAT 콘텐츠 밸런싱
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PythonCaller
participant ccat_select
participant py_ccat_select
participant core_ccat_select
PythonCaller->>ccat_select: 입력 배열과 theta0 전달
ccat_select->>py_ccat_select: 검증된 입력 전달
py_ccat_select->>core_ccat_select: CCAT 선택 요청
core_ccat_select-->>py_ccat_select: 선택 결과 반환
py_ccat_select-->>ccat_select: dictionary 변환
ccat_select-->>PythonCaller: selected, group, discrepancy, info 반환
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/mlsirm-core/src/exposure.rs (1)
1203-1214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
map_or패턴을is_none_or로 단순화 가능Clippy가 지적한 대로,
best.map_or(true, |(bd, _)| ...)형태(Line 1208, Line 1256 동일 패턴)는Option::is_none_or로 더 간결하게 표현할 수 있습니다.is_none_or는 Rust 1.82에서 안정화되었으므로, 프로젝트 MSRV가 이를 지원하는지 확인이 필요합니다.♻️ 제안 diff
- if eligible[g] && best.map_or(true, |(bd, _)| discrepancy[g] > bd) { + if eligible[g] && best.is_none_or(|(bd, _)| discrepancy[g] > bd) {Line 1256도 동일하게:
- if groups[i] == group && !administered[i] && best.map_or(true, |(bi, _)| info[i] > bi) { + if groups[i] == group && !administered[i] && best.is_none_or(|(bi, _)| info[i] > bi) {🤖 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 `@crates/mlsirm-core/src/exposure.rs` around lines 1203 - 1214, Update both discrepancy-selection conditions near the group-selection logic to use Option::is_none_or instead of best.map_or(true, ...), including the matching occurrence around the second referenced location. First confirm the project MSRV supports Rust 1.82; if not, retain the existing compatible pattern.Source: Linters/SAST tools
tests/unit/exposure_tests.rs (1)
1242-1317: 🩺 Stability & Availability | 🔵 Trivial
#[ignore]처리된 MC-500 불변식 테스트가 CI에서 실행되는지 확인 필요
ccat_mc500_invariants는 새 CCAT 규칙에 대한 유일한 랜덤화 불변식 검증인데#[ignore]로 표시되어 있어 기본cargo test실행 시 건너뜁니다. CI 파이프라인이cargo test -- --ignored(또는 동등한 단계)로 이 테스트를 실제로 실행하는지 확인해 주세요.🤖 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 `@tests/unit/exposure_tests.rs` around lines 1242 - 1317, Ensure the ignored ccat_mc500_invariants test is executed by CI by adding or updating the relevant test command to run cargo test with --ignored (or an equivalent dedicated step). Keep the test’s #[ignore] annotation and verify the CI configuration explicitly covers this randomized invariant test.
🤖 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.
Nitpick comments:
In `@crates/mlsirm-core/src/exposure.rs`:
- Around line 1203-1214: Update both discrepancy-selection conditions near the
group-selection logic to use Option::is_none_or instead of best.map_or(true,
...), including the matching occurrence around the second referenced location.
First confirm the project MSRV supports Rust 1.82; if not, retain the existing
compatible pattern.
In `@tests/unit/exposure_tests.rs`:
- Around line 1242-1317: Ensure the ignored ccat_mc500_invariants test is
executed by CI by adding or updating the relevant test command to run cargo test
with --ignored (or an equivalent dedicated step). Keep the test’s #[ignore]
annotation and verify the CI configuration explicitly covers this randomized
invariant test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d6a75b3-73ea-417d-9aae-158153609fde
📒 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 Kingsbury & Zara (1989) constrained CAT (CCAT) content-balanced item selection as a single-step
ccat_selectcompanion tokl_select, per the autonomous paper-implementation loop (iteration 23).Rule (verified contract)
t_g - k_g/k(target minus empirical proportion) wins; ties -> lowest index.a^2 (Q/P) ((P-c)/(1-c))^2attheta0is selected; ties -> lowest index.Returns
{selected, group, discrepancy (per group), info (per item, whole pool)}.Citation governance
nextItem.RcbControlbranch (READ during adversarial spec review), including the zero-empirical-proportion priority branch.Ii.R/Pi.R.Adversarial spec-verify (BEFORE implementation): GO-WITH-CHANGES
All changes applied: zero-coverage priority added, gap rule on proportions (not raw counts), k derived from the mask (no separate param), strictly positive targets summing to 1, info computed for the whole pool (masking applies to selection only).
Evidence
CcatSelectResultfields or returnedErrs.#[ignore]structural-invariant test over 500 random pools/masks: passes.py_ccat_select; Python wrapper validatesgroupsBEFORE the uintp cast (rejects negatives, non-integers, complex); pytestTestCcatSelect(4 tests, pass).Adversarial impl-review
Round 1 (1 MAJOR, fixed in 3c27789): with
c = 0and an extreme finitetheta0the logistic underflows toP = 0, making the naiveq/p * r^2info forminf * 0 = NaN, which silently corrupted the within-group argmax (x > NaNis false, so a NaN-info item stayed selected). Repro: poola=[1,1], b=[0,-1e308], c=[0,0],theta0=-1e308selected item 0 withinfo=[NaN, 0.25]instead of item 1. Fix: return the limiting value 0 whenp == 0(withc = 0,I = a^2 q p -> 0). Regression testccat_underflow_info_is_zero_not_nanreads crate outputs; guard-revert mutant executed and killed (restore green). Round 2 in progress.Round 2 (3 MAJOR, fixed in 4c81ea9): all against the naive
q/p * r^2info form / interimp == 0guard on extreme-but-valid inputs: (1) subnormalc = 5e-324with underflowed L still gaveinf * 0 = NaN; (2)a = 1e154overflowed info to spurious+infvia multiplication order and won the argmax (true value 1.2167807506233457); (3) the guard masked a genuinely informative extreme item (a = 1e162, true info 2.8223507304721003 > 2.25). Fix: log-space computation using the identityr = (P-c)/(1-c) = L = sigmoid(a(theta0-b)), soI = exp(2 ln a + ln(1-c) + ln(1-L) + 2 ln L - ln p)withln L = -softplus(-z);c = 0reduces exactly toI = a^2 L (1-L). Regression test pins 80-digit Decimal references at 1e-9 relative; pinned 1e-12 oracles unchanged; two more executed mutation kills (c==0 branch dropped, naive softplus). Round 3 in progress.Round 3: CLEAN. Reviewer re-ran all three round-2 repros through the Python wrapper (selections 1, 1, 0; finite info), swept 10k ordinary parameter combos against the naive form (max relative deviation 1.48e-10; 1.8e-12 in the well-conditioned central region), found no reachable NaN in a pathology sweep (only a documented genuine +inf for a = 1e200 at theta0 = b), and confirmed all Rust (7 + MC-500) and Python (4) tests pass. Final mutation-kill ledger: 7 executed kills, restore green after each.
Summary by CodeRabbit
신규 기능
ccat_selectAPI를 추가했습니다.문서
테스트