Skip to content

Fix ROC AUC computation for tied prediction scores - #374

Merged
Mec-iS merged 1 commit into
smartcorelib:developmentfrom
teddytennant:fix-auc-tied-scores
Jul 8, 2026
Merged

Fix ROC AUC computation for tied prediction scores#374
Mec-iS merged 1 commit into
smartcorelib:developmentfrom
teddytennant:fix-auc-tied-scores

Conversation

@teddytennant

Copy link
Copy Markdown
Contributor

Fixes #373

Checklist

  • My branch is up-to-date with development branch.
  • Everything works and tested on latest stable Rust.
  • Coverage and Linting have been applied

Current behaviour

metrics::auc::AUC::get_score (the Mann-Whitney/Wilcoxon rank-sum ROC AUC) mis-handles tied prediction scores. Ties are only rank-averaged when the equal scores happen to be adjacent in the input array; when equal scores are non-adjacent in the input they get distinct ranks, yielding a wrong AUC.

get_score builds label_idx = y_pred.argsort() and assigns ranks by sorted position, later pairing rank[i] with label_idx[i]. The tie-detection loop, however, compared scores in the original input order:

if i == n - 1 || y_pred.get(i) != y_pred.get(i + 1) { ... }

argsort() returns an index permutation and does not reorder y_pred, so y_pred.get(i) reads unsorted input while rank[i] corresponds to the i-th smallest score. Tied scores therefore only get the averaged rank when they are already adjacent in the input; otherwise they receive i + 1 instead of the averaged value and the AUC is wrong.

Concrete reproducer (y_true = [0, 1, 1], y_pred = [0.5, 0.9, 0.5]): the two positives score {0.9, 0.5} versus the negative's {0.5}, so pairwise ROC AUC crediting ties 0.5 is (1.0 + 0.5) / 2 = 0.75 (matches sklearn.metrics.roc_auc_score). Before this change the function returns 1.0.

New expected behaviour

The tie-detection compares scores in sorted order via label_idx, so equal scores are always grouped and rank-averaged regardless of their input position. label_idx[i + 1] is only read when i != n - 1, so there is no out-of-bounds access. The reproducer now returns 0.75.

Testing:

  • Added auc_tied_scores, a regression test using two tied, non-adjacent scores. It fails on the current code (returns 1.0) and passes with the fix (0.75).
  • The existing auc test still passes (its tie group is already sorted-adjacent, so ranks are unchanged).
  • cargo test --lib metrics::auc — 2 passed, 0 failed.
  • cargo fmt --all -- --check — clean.
  • cargo clippy --all-features -- -Drust-2018-idioms -Dwarnings — clean.

Change logs

Non-breaking bug fix (no interface change), so no CHANGELOG entry per the contributing guidelines (which ask to update CHANGELOG.md for breaking interface changes).

The Mann-Whitney/Wilcoxon rank-sum AUC assigns average ranks to tied
prediction scores. The tie-detection loop compared y_pred values in the
original input order (y_pred.get(i)) while the ranks it writes are indexed
by sorted position and later paired with label_idx. argsort() does not
mutate y_pred, so equal scores were only grouped when they happened to be
adjacent in the input array; non-adjacent ties received distinct ranks
instead of the averaged rank, producing an incorrect AUC.

Detect ties in sorted order via label_idx so equal scores are always
rank-averaged regardless of their input position.

Add a regression test with two tied, non-adjacent scores; it returns 0.75
(matching sklearn's roc_auc_score) with the fix and 1.0 without it.
@teddytennant
teddytennant requested a review from Mec-iS as a code owner July 8, 2026 17:36
Copilot AI review requested due to automatic review settings July 8, 2026 17:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Mec-iS

Mec-iS commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks for your PR!
Please correct the errors coming from the CI. After that I will proceed to a thorough PR review. Please consider adding at least one test to confirm the expected behaviour

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.20%. Comparing base (70d8a0f) to head (93ebc5e).
⚠️ Report is 18 commits behind head on development.

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #374      +/-   ##
===============================================
- Coverage        45.59%   44.20%   -1.40%     
===============================================
  Files               93       94       +1     
  Lines             8034     8052      +18     
===============================================
- Hits              3663     3559     -104     
- Misses            4371     4493     +122     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Mec-iS

Mec-iS commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

OK, there is already one test provide.

@teddytennant please consider a small change:
🔴 Minor: Bounds safety in the inner  while  loop
The inner loop  while j < n && y_pred.get(label_idx[j]) == ...  relies on  label_idx[j]  being a valid index into  y_pred . If  label_idx  is always a permutation of  [0..n)  (which  argsort()  guarantees), this is safe. However, this invariant is implicit and not enforced at the type level. A brief comment asserting this — or a  debug_assert!(label_idx[j] < n)  — would improve defensive correctness and readability.

After this is applied I will merge and it will go in the next release.

Mec-iS added a commit that referenced this pull request Jul 8, 2026
- Apply the core fix from PR #374: compare scores in sorted order
  via label_idx to correctly rank-average non-adjacent tied scores.
- Add `auc_all_tied_scores` edge-case test (all predictions equal → AUC 0.5).
- Add comments documenting the rank loop invariant and tie-group logic.
- Add `debug_assert!(label_idx[j] < n)` to make bounds safety explicit.
- Add comment on the rank-averaging formula.
@Mec-iS

Mec-iS commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

I will apply these changes in another branch. I will merge these.

fix-auc-review-followup

@Mec-iS
Mec-iS merged commit 938696a into smartcorelib:development Jul 8, 2026
11 of 13 checks passed
Mec-iS added a commit that referenced this pull request Jul 8, 2026
…ests, v0.6.0 (#375)

* fix(auc): apply PR #374 bug fix + review follow-ups

- Apply the core fix from PR #374: compare scores in sorted order
  via label_idx to correctly rank-average non-adjacent tied scores.
- Add `auc_all_tied_scores` edge-case test (all predictions equal → AUC 0.5).
- Add comments documenting the rank loop invariant and tie-group logic.
- Add `debug_assert!(label_idx[j] < n)` to make bounds safety explicit.
- Add comment on the rank-averaging formula.

* fix(series_encoder): replace sort_by with sort_by_key (clippy), add coverage tests

- Fix clippy::unnecessary_sort_by: use sort_by_key(|a| a.1) instead of
  sort_by(|a, b| a.1.cmp(&b.1)) in from_category_map.
- Add tests covering: num_categories(), get_num(), get_cat(),
  get_categories() slice, get_ordinal() for unknown category,
  invert_one_hot() multi-hot error path, and make_one_hot() directly.

* chore: bump version 0.5.0 -> 0.6.0

* chore: correct version bump 0.6.0 -> 0.5.1 (patch release)
@Mec-iS

Mec-iS commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

metrics::auc: tied prediction scores are not rank-averaged (wrong ROC AUC)

3 participants