Add Voice Arena rating engine (Davidson-BT) - #134
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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: defaults Review profile: CHILL Plan: Pro 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Pure-function rating engine for arena layer #3: Davidson Bradley-Terry MLE fit (theta + tie parameter nu), Elo conversion, percentile-bootstrap CIs, and a status classifier. Refuses a non-converged fit and skips non-converged bootstrap resamples. CI fields are null (not NaN) when unavailable. Unit tests cover ranking recovery, CI shrinkage with votes, and tie sensitivity.
seribaymadina
force-pushed
the
arena/03-rating-engine
branch
from
June 17, 2026 22:07
2d0b049 to
74ba8c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
runner/tests/unit/test_arena_rating.py (1)
128-225: ⚡ Quick winAdd regression tests for invalid topology/input paths.
Please add tests asserting rejection of disconnected comparison graphs and
model_a == model_boutcomes, so these correctness guards stay enforced.🤖 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 `@runner/tests/unit/test_arena_rating.py` around lines 128 - 225, Add two new test functions to the file to ensure invalid input scenarios are caught: first, create a test that verifies compute_ratings raises an appropriate error when given outcomes with model_a equal to model_b (self-comparison), and second, create a test that verifies compute_ratings raises an appropriate error when given a disconnected comparison graph where some models cannot be reached from others. Both tests should use pytest.raises to assert that the invalid inputs are properly rejected and the correctness guards remain enforced.
🤖 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 `@runner/src/coval_bench/arena/rating.py`:
- Around line 315-320: The condition checking for empty samples only handles the
case when vals.size equals zero, but when there is exactly one sample (vals.size
== 1), the code proceeds to calculate the 2.5th and 97.5th percentiles which
both return the same value, creating a false zero-width confidence interval with
ci_low == ci_high. Change the condition from checking if vals.size == 0 to
checking if vals.size < 2 so that cases with fewer than two samples are skipped
and treated as unavailable confidence intervals.
- Around line 163-170: Add a validation check before the pair aggregation logic
(before the line with lo, hi assignment) to reject outcomes where a model plays
against itself. Check if o.model_a equals o.model_b and skip processing that
outcome (continue to the next iteration) to prevent creating invalid self-pair
tuples like (k, k) in the pairs dictionary that would corrupt the statistics
tracking.
- Around line 364-370: The code aggregates outcomes using _aggregate but
immediately calls _fit without validating that the comparison graph is
connected. For disconnected components, cross-cluster Elo ordering is not
identified and becomes arbitrary. After the _aggregate call (which sets the
`agg` variable), add a connectivity check on the aggregated graph before
proceeding with the _fit call. If the graph is disconnected, return early from
the function with an appropriate result rather than attempting to fit the model
with disconnected clusters.
- Around line 351-357: The compute_ratings function does not validate its input
parameters bootstrap_rounds and reg, allowing negative values for
bootstrap_rounds to silently disable CI and negative or non-finite values for
reg to invalidate the optimization objective. Add validation logic at the
beginning of the compute_ratings function to ensure bootstrap_rounds is a
positive integer (greater than 0) and reg is a positive and finite number
(greater than 0 and not NaN or infinity), raising an appropriate exception if
either parameter is invalid.
---
Nitpick comments:
In `@runner/tests/unit/test_arena_rating.py`:
- Around line 128-225: Add two new test functions to the file to ensure invalid
input scenarios are caught: first, create a test that verifies compute_ratings
raises an appropriate error when given outcomes with model_a equal to model_b
(self-comparison), and second, create a test that verifies compute_ratings
raises an appropriate error when given a disconnected comparison graph where
some models cannot be reached from others. Both tests should use pytest.raises
to assert that the invalid inputs are properly rejected and the correctness
guards remain enforced.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5692fa35-6a25-46f5-8495-f1470baf7edf
📒 Files selected for processing (3)
runner/src/coval_bench/arena/__init__.pyrunner/src/coval_bench/arena/rating.pyrunner/tests/unit/test_arena_rating.py
Comment on lines
+163
to
+170
| a, b = index[o.model_a], index[o.model_b] | ||
| lo, hi = (a, b) if a < b else (b, a) | ||
| cell = pairs.setdefault((lo, hi), [0.0, 0.0, 0.0]) | ||
| if o.outcome == "TIE": | ||
| cell[2] += 1.0 | ||
| else: | ||
| winner = a if o.outcome == "A_WIN" else b | ||
| cell[0 if winner == lo else 1] += 1.0 |
There was a problem hiding this comment.
Reject self-battles before pair aggregation.
Line [164] allows model_a == model_b, which creates a (k, k) pair; then Line [170] collapses directionality and corrupts sufficient statistics. This should fail fast.
Suggested fix
def _aggregate(outcomes: Sequence[BattleOutcome]) -> _Aggregate:
"""Collapse raw battles into per-pair win/tie counts."""
models = sorted({o.model_a for o in outcomes} | {o.model_b for o in outcomes})
index = {m: k for k, m in enumerate(models)}
@@
pairs: dict[tuple[int, int], list[float]] = {}
for o in outcomes:
+ if o.model_a == o.model_b:
+ raise ValueError("BattleOutcome requires distinct model_a and model_b")
a, b = index[o.model_a], index[o.model_b]
lo, hi = (a, b) if a < b else (b, a)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| a, b = index[o.model_a], index[o.model_b] | |
| lo, hi = (a, b) if a < b else (b, a) | |
| cell = pairs.setdefault((lo, hi), [0.0, 0.0, 0.0]) | |
| if o.outcome == "TIE": | |
| cell[2] += 1.0 | |
| else: | |
| winner = a if o.outcome == "A_WIN" else b | |
| cell[0 if winner == lo else 1] += 1.0 | |
| if o.model_a == o.model_b: | |
| raise ValueError("BattleOutcome requires distinct model_a and model_b") | |
| a, b = index[o.model_a], index[o.model_b] | |
| lo, hi = (a, b) if a < b else (b, a) | |
| cell = pairs.setdefault((lo, hi), [0.0, 0.0, 0.0]) | |
| if o.outcome == "TIE": | |
| cell[2] += 1.0 | |
| else: | |
| winner = a if o.outcome == "A_WIN" else b | |
| cell[0 if winner == lo else 1] += 1.0 |
🤖 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 `@runner/src/coval_bench/arena/rating.py` around lines 163 - 170, Add a
validation check before the pair aggregation logic (before the line with lo, hi
assignment) to reject outcomes where a model plays against itself. Check if
o.model_a equals o.model_b and skip processing that outcome (continue to the
next iteration) to prevent creating invalid self-pair tuples like (k, k) in the
pairs dictionary that would corrupt the statistics tracking.
Comment on lines
+315
to
+320
| vals = np.array(samples[k], dtype=np.float64) | ||
| if vals.size == 0: | ||
| continue | ||
| ci_low[k] = float(np.percentile(vals, 2.5)) | ||
| ci_high[k] = float(np.percentile(vals, 97.5)) | ||
| return ci_low, ci_high |
There was a problem hiding this comment.
Avoid zero-width CI from a single bootstrap sample.
Line [318] and Line [319] run percentile on vals.size == 1, yielding ci_low == ci_high and a false “established” half-width of 0. Treat fewer than two samples as unavailable CI.
Suggested fix
for k in range(n):
vals = np.array(samples[k], dtype=np.float64)
- if vals.size == 0:
+ if vals.size < 2:
continue
ci_low[k] = float(np.percentile(vals, 2.5))
ci_high[k] = float(np.percentile(vals, 97.5))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vals = np.array(samples[k], dtype=np.float64) | |
| if vals.size == 0: | |
| continue | |
| ci_low[k] = float(np.percentile(vals, 2.5)) | |
| ci_high[k] = float(np.percentile(vals, 97.5)) | |
| return ci_low, ci_high | |
| vals = np.array(samples[k], dtype=np.float64) | |
| if vals.size < 2: | |
| continue | |
| ci_low[k] = float(np.percentile(vals, 2.5)) | |
| ci_high[k] = float(np.percentile(vals, 97.5)) | |
| return ci_low, ci_high |
🤖 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 `@runner/src/coval_bench/arena/rating.py` around lines 315 - 320, The condition
checking for empty samples only handles the case when vals.size equals zero, but
when there is exactly one sample (vals.size == 1), the code proceeds to
calculate the 2.5th and 97.5th percentiles which both return the same value,
creating a false zero-width confidence interval with ci_low == ci_high. Change
the condition from checking if vals.size == 0 to checking if vals.size < 2 so
that cases with fewer than two samples are skipped and treated as unavailable
confidence intervals.
Comment on lines
+351
to
+357
| def compute_ratings( | ||
| outcomes: Sequence[BattleOutcome], | ||
| *, | ||
| bootstrap_rounds: int = 1000, | ||
| seed: int = 0, | ||
| reg: float = 0.1, | ||
| ) -> RatingResult: |
There was a problem hiding this comment.
Validate bootstrap_rounds and reg inputs.
Line [354] and Line [356] accept invalid values currently: negative bootstrap_rounds silently disables CI, and negative/non-finite reg can invalidate the optimization objective.
Suggested fix
def compute_ratings(
@@
) -> RatingResult:
@@
+ if bootstrap_rounds < 0:
+ raise ValueError("bootstrap_rounds must be >= 0")
+ if not math.isfinite(reg) or reg < 0.0:
+ raise ValueError("reg must be finite and >= 0")
+
agg = _aggregate(outcomes)🤖 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 `@runner/src/coval_bench/arena/rating.py` around lines 351 - 357, The
compute_ratings function does not validate its input parameters bootstrap_rounds
and reg, allowing negative values for bootstrap_rounds to silently disable CI
and negative or non-finite values for reg to invalidate the optimization
objective. Add validation logic at the beginning of the compute_ratings function
to ensure bootstrap_rounds is a positive integer (greater than 0) and reg is a
positive and finite number (greater than 0 and not NaN or infinity), raising an
appropriate exception if either parameter is invalid.
Comment on lines
+364
to
+370
| agg = _aggregate(outcomes) | ||
| n = len(agg.models) | ||
| if n == 0: | ||
| return RatingResult(tie_param=0.0, models=[]) | ||
|
|
||
| theta, nu = _fit(agg, reg) | ||
| elo = _elo(theta) |
There was a problem hiding this comment.
Guard against disconnected comparison graphs before fitting.
Line [364] aggregates outcomes, but Line [369] fits immediately without connectivity validation. For disconnected components, cross-cluster Elo ordering is not identified and can be arbitrarily ridge-pinned.
Suggested fix
+def _assert_connected(agg: _Aggregate) -> None:
+ n = len(agg.models)
+ if n <= 1:
+ return
+ adj: list[list[int]] = [[] for _ in range(n)]
+ for a, b in zip(agg.idx_i, agg.idx_j, strict=False):
+ if a == b:
+ continue
+ adj[int(a)].append(int(b))
+ adj[int(b)].append(int(a))
+
+ seen = {0}
+ stack = [0]
+ while stack:
+ u = stack.pop()
+ for v in adj[u]:
+ if v not in seen:
+ seen.add(v)
+ stack.append(v)
+ if len(seen) != n:
+ raise ConvergenceError("comparison graph is disconnected; cross-component strengths are undefined")
@@
def compute_ratings(
@@
agg = _aggregate(outcomes)
n = len(agg.models)
if n == 0:
return RatingResult(tie_param=0.0, models=[])
+ _assert_connected(agg)
theta, nu = _fit(agg, reg)🤖 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 `@runner/src/coval_bench/arena/rating.py` around lines 364 - 370, The code
aggregates outcomes using _aggregate but immediately calls _fit without
validating that the comparison graph is connected. For disconnected components,
cross-cluster Elo ordering is not identified and becomes arbitrary. After the
_aggregate call (which sets the `agg` variable), add a connectivity check on the
aggregated graph before proceeding with the _fit call. If the graph is
disconnected, return early from the function with an appropriate result rather
than attempting to fit the model with disconnected clusters.
Skip self-battle outcomes in aggregation, treat fewer than two bootstrap samples as no CI (avoids a false zero-width 'established' interval), and validate bootstrap_rounds/reg. Document the disconnected-graph limitation and that adaptive pairing is the intended upstream remedy.
Cover the self-battle skip, single-sample CI falling back to preliminary, and the bootstrap_rounds/reg input validation.
seribaymadina
added a commit
that referenced
this pull request
Jun 19, 2026
* Add Voice Arena rating engine (Davidson-BT) Pure-function rating engine for arena layer #3: Davidson Bradley-Terry MLE fit (theta + tie parameter nu), Elo conversion, percentile-bootstrap CIs, and a status classifier. Refuses a non-converged fit and skips non-converged bootstrap resamples. CI fields are null (not NaN) when unavailable. Unit tests cover ranking recovery, CI shrinkage with votes, and tie sensitivity. * Harden arena rating engine inputs and CI Skip self-battle outcomes in aggregation, treat fewer than two bootstrap samples as no CI (avoids a false zero-width 'established' interval), and validate bootstrap_rounds/reg. Document the disconnected-graph limitation and that adaptive pairing is the intended upstream remedy. * Add regression tests for rating engine hardening Cover the self-battle skip, single-sample CI falling back to preliminary, and the bootstrap_rounds/reg input validation.
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jun 19, 2026
* Add Voice Arena rating engine (Davidson-BT) (#134) * Add Voice Arena rating engine (Davidson-BT) Pure-function rating engine for arena layer #3: Davidson Bradley-Terry MLE fit (theta + tie parameter nu), Elo conversion, percentile-bootstrap CIs, and a status classifier. Refuses a non-converged fit and skips non-converged bootstrap resamples. CI fields are null (not NaN) when unavailable. Unit tests cover ranking recovery, CI shrinkage with votes, and tie sensitivity. * Harden arena rating engine inputs and CI Skip self-battle outcomes in aggregation, treat fewer than two bootstrap samples as no CI (avoids a false zero-width 'established' interval), and validate bootstrap_rounds/reg. Document the disconnected-graph limitation and that adaptive pairing is the intended upstream remedy. * Add regression tests for rating engine hardening Cover the self-battle skip, single-sample CI falling back to preliminary, and the bootstrap_rounds/reg input validation. * Treat zero-variance bootstrap as no CI; require reg > 0 A fully-separated dataset refits identically every resample, giving a zero-width interval that wrongly read as 'established'; skip it so it reads preliminary. Also require reg > 0, since the ridge the engine relies on for convergence is absent at reg = 0.
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.
Rating engine for arena layer #3, stacked on the schema PR (#119).
Pure functions, no DB/IO: A_WIN/B_WIN/TIE outcomes in, one rating row per model out (matching arena.leaderboard_snapshots).
Tests cover the three done-criteria (recovers a known ranking, CIs shrink with votes, ties move nu) plus the convergence and null-CI paths.
Follow-ups (not this PR): calibrate
regand pin/decouple the status thresholds once methodology lands, per-domain tie rate (far future).Merge after #119.
Summary by CodeRabbit
New Features
Tests
Greptile Summary
Adds the Davidson Bradley-Terry rating engine for the Voice Arena leaderboard layer. Pure-function implementation: battle outcomes in, one
ModelRatingrow per model out, matching thearena.leaderboard_snapshotsschema from #119.ConvergenceErrorguard so a non-converged optimizer never produces ratings.Noneconversion (no silent poison values in the schema), and aclassify_statustier based on CI half-width.Confidence Score: 5/5
Safe to merge; this is a pure-function module with no DB/IO and a well-guarded optimizer path.
All three blocking concerns from the previous round (optimizer convergence check, NaN to None schema correctness, and the CI containment assertion) are fully addressed. The two remaining observations are methodological trade-offs explicitly acknowledged in the PR description and docstrings.
No files require special attention; rating.py is the only complex file and its core MLE and bootstrap paths are well-tested.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["compute_ratings(outcomes)"] --> B["_aggregate(outcomes)\nper-pair win/tie counts"] B --> C{n == 0?} C -- yes --> D["RatingResult(models=[])"] C -- no --> E["_fit(agg, reg)\nL-BFGS-B MLE"] E --> F{res.success?} F -- no --> G["raise ConvergenceError"] F -- yes --> H["recenter theta\nnu = exp(eta)"] H --> I["_elo(theta)\n1500 + scale * theta"] I --> J["_bootstrap(outcomes, models)\nresample battles x rounds"] J --> K{replicate converges?} K -- no --> L["skip replicate\nlog warning"] K -- yes --> M["collect Elo\nper model"] L --> N{more rounds?} M --> N N -- yes --> J N -- no --> O["percentile 2.5/97.5\n(need >= 2 samples)"] O --> P["nan to None\ncompute half_width"] P --> Q["classify_status\npreliminary/usable/established"] Q --> R["sort by Elo desc\nRatingResult"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A["compute_ratings(outcomes)"] --> B["_aggregate(outcomes)\nper-pair win/tie counts"] B --> C{n == 0?} C -- yes --> D["RatingResult(models=[])"] C -- no --> E["_fit(agg, reg)\nL-BFGS-B MLE"] E --> F{res.success?} F -- no --> G["raise ConvergenceError"] F -- yes --> H["recenter theta\nnu = exp(eta)"] H --> I["_elo(theta)\n1500 + scale * theta"] I --> J["_bootstrap(outcomes, models)\nresample battles x rounds"] J --> K{replicate converges?} K -- no --> L["skip replicate\nlog warning"] K -- yes --> M["collect Elo\nper model"] L --> N{more rounds?} M --> N N -- yes --> J N -- no --> O["percentile 2.5/97.5\n(need >= 2 samples)"] O --> P["nan to None\ncompute half_width"] P --> Q["classify_status\npreliminary/usable/established"] Q --> R["sort by Elo desc\nRatingResult"]Reviews (3): Last reviewed commit: "Add regression tests for rating engine h..." | Re-trigger Greptile