quality() and predict_quality() panic for anything other than exactly 2 rating groups #9

New Issue

2026-08-04T19:14:46Z

logaritmisk commented

2026-08-04 19:14:46 +00:00

quality() is documented and exported as a general N-group match-quality function (src/lib.rs:199), but it only works for exactly two groups. Verified on main @ 2b5d3b1, --release:

quality(&[&[a], &[b]],      beta)   // ok -> 0.8115...
quality(&[&[a], &[b], &[c]], beta)  // PANIC: index out of bounds: the len is 3 but the index is 3
quality(&[&[a]],             beta)  // PANIC: "eh, okey"

History::predict_quality calls straight through (src/history.rs:405), so it panics identically:

h.predict_quality(&[&[&"a"], &[&"b"], &[&"c"]])  // PANIC at src/lib.rs:240

Three separate defects.

1. The `x` column counter is wrong (`src/lib.rs:224-244`)

let mut t = 0;
let mut x = 0;
for (row, group) in rating_groups.windows(2).enumerate() {
    for n in t..t + current.len() {
        rotated_a_matrix[(row, n)] = flatten_weights[n];
        x += 1;                       // <-- x tracks the +1 block...
    }
    t += current.len();
    for n in x..x + next.len() {      // <-- ...but is then reused as the -1 block start
        rotated_a_matrix[(row, n)] = -flatten_weights[n];
    }
    x += next.len();
}

x accumulates across rows while t restarts the window each iteration, so from row 1 onward the two counters disagree. With three singleton groups: row 1 writes +1 at column 1 (correct) then attempts -1 at column 3 of a 2×3 matrix → out of bounds. For two groups the loop body runs once and x never diverges from t, which is why the only existing test (test_quality, src/lib.rs:273) passes.

The -1 block should start at t + current.len() — the column right after the +1 block — making the separate x counter unnecessary.

2. `Matrix::inverse` is unimplemented for n > 1 (`src/matrix.rs:112-122`)

pub fn inverse(&self) -> Matrix {
    let mut matrix = Matrix::new(self.width, self.height);
    if self.height == self.width && self.height == 1 {
        matrix[(0, 0)] = 1.0 / self[(0, 0)];
    } else {
        panic!("eh, okey")
    }
    matrix
}

quality() calls middle.inverse() where middle is (k-1)×(k-1) for k groups. Only k == 2 yields a 1×1 matrix. For k >= 3 this panic is reached the moment defect 1 is fixed, and for k == 1 it is reached immediately (0×0 matrix).

Matrix::adjugate (src/matrix.rs:89) computes exactly what a cofactor inverse needs and is currently dead code — nothing calls it. Wiring inverse to adjugate / determinant would work for small n, though see defect 3.

3. `det()` is O(n!) (`src/matrix.rs:3-25`)

Recursive cofactor expansion, allocating a fresh Vec per minor per level. Fine at n=1..2; unusable beyond ~n=8. quality() on a 10-team match would need a 9×9 determinant — 362,880 recursive terms with an allocation each.

If N-group quality is worth supporting, Matrix needs an LU (or Cholesky, since the matrices here are symmetric positive-definite by construction) factorization for both determinant and inverse. That also removes the adjugate/minor cofactor path entirely.

Also worth fixing while here

predict_quality silently drops unknown keys. Both filter_maps in src/history.rs:393-400 discard keys that aren't interned and competitors with no posterior. Querying a team of entirely-unknown players yields an empty group and a plausible-looking number rather than an error — measured: h.predict_quality(&[&[&"a"], &[&"nobody"]]) returns 0.1585… instead of signalling that half the query was meaningless.
Matrix has no dimension checks. Mul/Add (src/matrix.rs:155-213) assume conformable shapes and will silently read wrong elements or panic on OOB.
Over-constrained lifetimes. impl<'a> ops::Mul<&'a Matrix> for &'a Matrix (src/matrix.rs:177) forces both operands to share one lifetime; they should be independent.

Scope

Fix the column indexing in quality().
Implement Matrix::inverse for n > 1, or replace the hand-rolled Matrix with a factorization-based implementation.
Define and enforce the behaviour for degenerate inputs: 0 groups, 1 group, empty groups.
Make predict_quality surface unknown keys rather than silently dropping them.

Acceptance

quality() returns finite, correct values for 2, 3, 5, and 10 groups, cross-checked against sublee/trueskill (this is the open README Todo item — see #7).
Degenerate inputs return an error or a documented value; no panics.
predict_quality covered by tests at 3+ teams.

`quality()` is documented and exported as a general N-group match-quality function (`src/lib.rs:199`), but it only works for exactly two groups. Verified on `main` @ 2b5d3b1, `--release`: ```rust quality(&[&[a], &[b]], beta) // ok -> 0.8115... quality(&[&[a], &[b], &[c]], beta) // PANIC: index out of bounds: the len is 3 but the index is 3 quality(&[&[a]], beta) // PANIC: "eh, okey" ``` `History::predict_quality` calls straight through (`src/history.rs:405`), so it panics identically: ```rust h.predict_quality(&[&[&"a"], &[&"b"], &[&"c"]]) // PANIC at src/lib.rs:240 ``` Three separate defects. ## 1. The `x` column counter is wrong (`src/lib.rs:224-244`) ```rust let mut t = 0; let mut x = 0; for (row, group) in rating_groups.windows(2).enumerate() { for n in t..t + current.len() { rotated_a_matrix[(row, n)] = flatten_weights[n]; x += 1; // <-- x tracks the +1 block... } t += current.len(); for n in x..x + next.len() { // <-- ...but is then reused as the -1 block start rotated_a_matrix[(row, n)] = -flatten_weights[n]; } x += next.len(); } ``` `x` accumulates across rows while `t` restarts the window each iteration, so from row 1 onward the two counters disagree. With three singleton groups: row 1 writes `+1` at column 1 (correct) then attempts `-1` at column 3 of a 2×3 matrix → out of bounds. For two groups the loop body runs once and `x` never diverges from `t`, which is why the only existing test (`test_quality`, `src/lib.rs:273`) passes. The `-1` block should start at `t + current.len()` — the column right after the `+1` block — making the separate `x` counter unnecessary. ## 2. `Matrix::inverse` is unimplemented for n > 1 (`src/matrix.rs:112-122`) ```rust pub fn inverse(&self) -> Matrix { let mut matrix = Matrix::new(self.width, self.height); if self.height == self.width && self.height == 1 { matrix[(0, 0)] = 1.0 / self[(0, 0)]; } else { panic!("eh, okey") } matrix } ``` `quality()` calls `middle.inverse()` where `middle` is `(k-1)×(k-1)` for `k` groups. Only `k == 2` yields a 1×1 matrix. For `k >= 3` this panic is reached the moment defect 1 is fixed, and for `k == 1` it is reached immediately (0×0 matrix). `Matrix::adjugate` (`src/matrix.rs:89`) computes exactly what a cofactor inverse needs and is currently **dead code** — nothing calls it. Wiring `inverse` to `adjugate / determinant` would work for small n, though see defect 3. ## 3. `det()` is O(n!) (`src/matrix.rs:3-25`) Recursive cofactor expansion, allocating a fresh `Vec` per minor per level. Fine at n=1..2; unusable beyond ~n=8. `quality()` on a 10-team match would need a 9×9 determinant — 362,880 recursive terms with an allocation each. If N-group quality is worth supporting, `Matrix` needs an LU (or Cholesky, since the matrices here are symmetric positive-definite by construction) factorization for both `determinant` and `inverse`. That also removes the `adjugate`/`minor` cofactor path entirely. ## Also worth fixing while here - **`predict_quality` silently drops unknown keys.** Both `filter_map`s in `src/history.rs:393-400` discard keys that aren't interned and competitors with no posterior. Querying a team of entirely-unknown players yields an empty group and a plausible-looking number rather than an error — measured: `h.predict_quality(&[&[&"a"], &[&"nobody"]])` returns `0.1585…` instead of signalling that half the query was meaningless. - **`Matrix` has no dimension checks.** `Mul`/`Add` (`src/matrix.rs:155-213`) assume conformable shapes and will silently read wrong elements or panic on OOB. - **Over-constrained lifetimes.** `impl<'a> ops::Mul<&'a Matrix> for &'a Matrix` (`src/matrix.rs:177`) forces both operands to share one lifetime; they should be independent. ## Scope - Fix the column indexing in `quality()`. - Implement `Matrix::inverse` for n > 1, or replace the hand-rolled `Matrix` with a factorization-based implementation. - Define and enforce the behaviour for degenerate inputs: 0 groups, 1 group, empty groups. - Make `predict_quality` surface unknown keys rather than silently dropping them. ## Acceptance - `quality()` returns finite, correct values for 2, 3, 5, and 10 groups, cross-checked against [sublee/trueskill](https://github.com/sublee/trueskill) (this is the open README Todo item — see #7). - Degenerate inputs return an error or a documented value; no panics. - `predict_quality` covered by tests at 3+ teams.

logaritmisk referenced this issue

2026-08-04 19:19:55 +00:00

Public API gaps: `predict_outcome` panics and is 2-team-only, `HistoryBuilder` isn't exported, `Rating`/`Index` are write-only #21

logaritmisk referenced this issue

2026-08-04 19:20:44 +00:00

Dead and duplicated code: a stale `iterate_to_convergence`, a copy-pasted sweep, and `#[allow(dead_code)]` used as a mute button #23

logaritmisk referenced this issue

2026-08-04 19:21:02 +00:00

No CI — nothing verifies builds, tests, feature combinations, or determinism #24

logaritmisk referenced this issue

2026-08-04 19:21:40 +00:00

Test suite has no coverage for degenerate inputs, release-mode behaviour, or non-finite results #26

logaritmisk commented

2026-08-04 20:11:20 +00:00

Fixed in 0d32690.

All three defects addressed:

The contrast-matrix column counter is gone — the negative block always starts immediately after the positive one, so the second counter was never needed.
Matrix::determinant and Matrix::inverse now share one LU decomposition with partial pivoting, replacing both the O(n!) cofactor expansion and panic!("eh, okey"). The dead adjugate/minor path is removed, and multiply/add gained dimension checks.
Degenerate inputs assert with a message naming the requirement rather than underflowing or producing NaN.

The two-group golden is unchanged. N-group behaviour is pinned by invariants in tests/quality.rs — permutation invariance, quality falling as a skill gap widens, finite and in-range for 2..=8 groups, uneven group sizes, and predict_quality at three teams.

Not done: the sublee/trueskill reference cross-check from the acceptance criteria. No reference values were available to compare against, so the N-group results are verified structurally rather than numerically. That remains the one open item in the README Todo list, and is the right place to track it.

predict_quality now documents that it panics on unknown keys rather than silently dropping them into an empty group — a loud failure instead of a confident wrong number, though returning Result would be better still (see #21).

Fixed in 0d32690. All three defects addressed: 1. The contrast-matrix column counter is gone — the negative block always starts immediately after the positive one, so the second counter was never needed. 2. `Matrix::determinant` and `Matrix::inverse` now share one LU decomposition with partial pivoting, replacing both the O(n!) cofactor expansion and `panic!("eh, okey")`. The dead `adjugate`/`minor` path is removed, and multiply/add gained dimension checks. 3. Degenerate inputs assert with a message naming the requirement rather than underflowing or producing NaN. The two-group golden is unchanged. N-group behaviour is pinned by invariants in `tests/quality.rs` — permutation invariance, quality falling as a skill gap widens, finite and in-range for 2..=8 groups, uneven group sizes, and `predict_quality` at three teams. **Not done:** the sublee/trueskill reference cross-check from the acceptance criteria. No reference values were available to compare against, so the N-group results are verified structurally rather than numerically. That remains the one open item in the README Todo list, and is the right place to track it. `predict_quality` now documents that it panics on unknown keys rather than silently dropping them into an empty group — a loud failure instead of a confident wrong number, though returning `Result` would be better still (see #21).

logaritmisk referenced this issue

2026-08-04 20:12:26 +00:00

README Todo list and CLAUDE.md architecture section are stale post-redesign #7

logaritmisk referenced this issue

2026-08-04 20:13:12 +00:00

Dead and duplicated code: a stale `iterate_to_convergence`, a copy-pasted sweep, and `#[allow(dead_code)]` used as a mute button #23

logaritmisk closed this issue

2026-08-04 20:13:32 +00:00

logaritmisk added the api bug labels 2026-09-07 13:53:08 +00:00

logaritmisk referenced this issue from a commit

2026-09-07 19:46:54 +00:00

test: pin quality()'s N-group closed form, closing the README cross-check