Add bratt_mm: Bradley-Terry model with additive ties parameter (VGAM bratt) - #307
Closed
seonghobae wants to merge 5 commits into
Closed
Add bratt_mm: Bradley-Terry model with additive ties parameter (VGAM bratt)#307seonghobae wants to merge 5 commits into
seonghobae wants to merge 5 commits into
Conversation
…ting) Rust core predict_rating_two/predict_rating_multi in mlsirm-core scaling (Elo logistic, deviation-shrunk Glicko-family with qip3 = 3(ln10/400/pi)^2, EloM rowmean branch with optional min-tie placing), PyO3 bindings, Python wrappers, exact-oracle anchor tests P1-P9, 7-mutant EXECUTED kill map, MC-500 invariants, TestPredict pytest coverage, CHANGELOG entry. Normative source: CRAN PlayerRatings 1.1-0 R/ratings.R lines 1056-1133 (READ). REDUCED-SCOPE: index-based players, per-game/scalar gamma. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Impl-review round-1 findings: (1) games/tng went through a float round-trip in the Python wrappers, losing exact integer counts at or above 2^53 and silently shifting the strict games < tng cutoff — new _predict_games_u64/_predict_tng_u64 keep integer inputs lossless and reject float inputs at/above the source dtype's exact-integer bound; (2) predict_rating_multi computed nr*np unchecked, so a wrapping product could pass the length check and panic on indexing — now checked_mul with a checked error. Regression tests at both the Rust error contract and pytest levels. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Implements the VGAM bratt() family model (VGAM 1.1-14 R/family.categorical.R, READ and normative): P(i beats j) = alpha_i / (alpha_i + alpha_j + alpha0), P(tie) = alpha0 / (...), fitted by a hand-derived supporting-hyperplane MM ascent with a joint likelihood-preserving rescale of alpha AND alpha0. This is the additive-alpha0 ties model, NOT Rao-Kupper or Davidson. - Rust core bratt_mm + BrattResult in mlsirm-core scaling.rs with full error contract (n cap 10000 before O(n^2), symmetric ties, zero-win and tie-free rejection directing to bradley_terry_mm). - bt2_ test block: exact-Fraction oracle anchors B1-B4 (iter-1 pins [1, 27/40, 3/4], alpha0 = 9/14; converged pins at 1e-13 with an independent spec-gradient check), permutation equivariance, rescale anchor, 14-path error contract, and an MC-500 log-likelihood dominance test (#[ignore]). - 5 mutation kills EXECUTED (W-includes-ties, alpha0 denominator double-count, rescale-skips-alpha0, D-missing-alpha0, and an alpha0-blind convergence check killed by a tol-separated convergence anchor pinning iterations == 2). - PyO3 binding bratt_mm + Python wrapper/BrattResult dataclass with hardened input validation; TestBratt (4 tests). cargo test -p mlsirm-core --lib: 840 passed. pytest tests/test_paper_features.py: 333 passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- MAJOR: finite-but-huge counts could overflow derived aggregates (w_tot, t_tot, pair totals n_ij) to +inf, letting the MM update return exactly-zero parameters and a NaN log-likelihood as Ok. Now every derived aggregate is checked for finiteness up front, updated parameters must be finite AND strictly positive, and a non-finite log-likelihood is rejected. Regression bt2_huge_counts_overflow_rejected covers both the pair-total and row-total overflow paths (asserts read the crate Err). - MINOR: object-dtype arrays of Python bools bypassed the boolean rejection at the Python boundary (cast cleanly to float64). The object path now rejects bool/np.bool_ elements before casting; regression added to TestBratt.test_validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…nt (#308) * Add fleiss_kappa: Fleiss' multi-rater kappa with exact (Conger) variant Reimplements CRAN irr 0.85 kappam.fleiss() (R source READ and normative; Fleiss 1971 and Conger 1980 NOT READ, cited as model origins only) in the Rust core (mlsirm_core::agreement::fleiss_kappa) with a thin PyO3 binding and NumPy wrapper: - classification-table agreement, classic (sum p_j^2) and exact (sum p_j^2 - (1/nr) sum s2_j) chance agreement; kappa, Fleiss' z test, and category-wise kappas (classic mode; NaN for empty categories, matching R's 0/0) - listwise row drop for missing ratings (negative code / NaN) - documented API deviations: index codes 0..k-1 with explicit/inferred k, error on degenerate 1 - chanceP = 0 (R returns NaN), size caps Evidence: exact-Fraction oracle anchors FK1-FK5 (classic kappa 139/399, exact 37/102, category kappas [1/21, 31/91, 43/63]); 6 mutants EXECUTED and killed (agreeP centering, row-vs-column chance sums, exact==classic, variance sign, missing-as-category, pjk centering); MC-500 subject/rater permutation-invariance test (#[ignore], executed); cargo 847 pass; TestFleiss pytest pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(fleiss): reject uint64 overflow and lossy k coercion in wrapper Impl-review round 1 findings: - MAJOR: uint64 values above i64::MAX wrapped negative via astype(int64) and were silently dropped as missing; now rejected before conversion. - MINOR: explicit k accepted lossy coercions (3.9, '3', bool); now requires a true integer (int or np.integer, bool excluded). Regression tests added to TestFleiss.test_validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
3 tasks
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements
bratt_mm— the Bradley-Terry model with an additive ties parameter alpha0, as specified by the VGAMbratt()family (VGAM 1.1-14,R/family.categorical.R— READ and normative source):Fitted by a hand-derived supporting-hyperplane MM ascent (same derivation pattern as the crate
s existingbradley_terry_mm) with a **joint** reference rescale of alpha AND alpha0 each iteration — likelihood-preserving via the verified identitysum(wins) + T = sum_{i<j} n_ij`. This is the additive-alpha0 ties model, NOT Rao-Kupper or Davidson (neither read; named for disambiguation only). Bradley & Terry (1952) NOT READ — cited as model origin only.Stacked on #306 (
seonghobae-predict).Spec-verify (adversarial, BEFORE implementation)
Verdict: APPROVED-WITH-CHANGES — all 5 findings adopted:
ties <- 0*y).s ownbradley_terry_mm`.Reviewer independently verified the scale-invariance identity and hand-checked the B1 anchor arithmetic.
Oracle (exact Fractions, EXECUTED)
files/bratt_oracle.py— all anchors pass:Mutation kills (all EXECUTED against the real crate)
MU5 note: symmetric-wins fixtures are provably blind to this mutant (the joint rescale fixes alpha0 = 0.5 after one update); the kill uses a tolerance placed strictly between the two deltas instead.
Tests
bt2_block: 7 tests +bt2_mc_500_dominance(#[ignore], 500-rep MC log-likelihood dominance over 20 perturbations with an independent LL-field recomputation cross-check). Every assert reads crate outputs.TestBratt: 4 tests (oracle pins through the PyO3 binding, validation rejects).Suite evidence
cargo test -p mlsirm-core --lib: 840 passed (833 + 7 new)pytest tests/test_paper_features.py -q: 333 passed (329 + 4 new)bt2_mc_500_dominance: passed (run explicitly)Adversarial impl-review outcome
Round 1: FINDINGS (2) ? both fixed in commit 9e32315:
w_tot,t_tot, pair totalsn_ij) to +inf, letting the MM update return exactly-zero parameters and a NaN log-likelihood asOk. Fixed with up-front finiteness checks on all derived aggregates, a strictly-positive requirement on updated parameters, and a log-likelihood finiteness check. Regressionbt2_huge_counts_overflow_rejectedcovers both the pair-total and row-total overflow paths.bool/np.bool_elements before casting; regression added toTestBratt.test_validation.Round 2: CLEAN (0 findings) ? round-1 demos now raise
ValueError;bt2_8 passed / 1 ignored;TestBratt4 passed; adversarial probes for residual overflow paths (denominator accumulation, LL accumulation, Decimal/Fraction/None object arrays, 0-d arrays) found no confirmed defects.