Skip to content

1200: Fix predicted_label handling and n > 1 column check in categorical metrics#1203

Open
nikosbosse wants to merge 1 commit into
mainfrom
issue-1200-categorical-label-handling
Open

1200: Fix predicted_label handling and n > 1 column check in categorical metrics#1203
nikosbosse wants to merge 1 commit into
mainfrom
issue-1200-categorical-label-handling

Conversation

@nikosbosse

Copy link
Copy Markdown
Collaborator

Description

This PR closes #1200.

Three related bugs in the categorical (nominal/ordinal) metrics when called directly with a predicted_label that is not in factor level order (forecasts scored via score() were unaffected, as the pipeline sorts predictions into level order before calling the metrics):

  • rps_ordinal() reordered the columns of predicted with the forward permutation predicted[, as.numeric(predicted_label)] instead of the inverse predicted[, order(as.numeric(predicted_label))], so scores were not invariant to how the columns were labelled.
  • logs_categorical() ignored predicted_label entirely and indexed predicted[cbind(1:n, as.numeric(observed))], returning the negative log of the probability of the wrong outcome for permuted labels.
  • assert_input_categorical() did not check the number of columns of predicted in the n > 1 branch (the n == 1 branch already did), so e.g. a 3x4 matrix with rows summing to 1 was silently accepted and scored meaninglessly.

The fix reorders predicted with order(as.numeric(predicted_label)) in both metrics (preserving NA-observed and single-observation behaviour) and adds ncols = N to the n > 1 assert_matrix() call. Note the latter is a new error on previously (silently, wrongly) accepted input. An existing test expectation in test-metrics-ordinal.R encoded the wrong permutation and has been corrected; permutation-invariance tests and a wrong-width-matrix error test have been added.

Tests were written first and failed against the unfixed code:

FAILURE: 'test-metrics-nominal.R:107:3' -----------
Expected `assert_input_nominal(observed, predicted_wide, predicted_label)` to throw a error.

FAILURE: 'test-metrics-nominal.R:162:3' -----------
Expected `res` to equal `logs_categorical(observed, predicted, predicted_label)`.
Differences:
  `actual`: 2.30 0.36 1.61
`expected`: 0.22 1.61 0.92

FAILURE: 'test-metrics-ordinal.R:138:3' -----------
Expected `rps_ordinal(observed, predicted[, perm], predicted_label2)` to equal `rps_ordinal(observed, predicted, predicted_label)`.
Differences:
  `actual`: 0.820000000 0.530000000 0.200000000
`expected`: 0.050000000 0.500000000 0.200000000

After the fix, the affected test files and the full test suite pass (0 failed, 910 passed).

Dev note: bug identified by an LLM audit (#1189); fix and tests implemented with LLM support, directed by @nikosbosse.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title as follows: issue-number: PR title
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required. (N/A — no roxygen/documentation changes were needed; the documented behaviour is unchanged.)
  • I have built the package locally and run rebuilt docs using roxygen2. (N/A — no roxygen comments were changed, so no man/ files needed rebuilding.)
  • My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

rps_ordinal() applied the forward instead of the inverse permutation when
reordering predicted columns, and logs_categorical() ignored
predicted_label entirely, so both returned wrong scores when
predicted_label was not in factor level order. Both now reorder columns
with order(as.numeric(predicted_label)) so scores are invariant to column
labelling. assert_input_categorical() now also checks the number of
columns of predicted for n > 1, matching the existing n == 1 check.
Corrects an existing test expectation that encoded the wrong permutation
and adds permutation-invariance tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.15%. Comparing base (06ed56d) to head (26da703).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1203   +/-   ##
=======================================
  Coverage   98.15%   98.15%           
=======================================
  Files          41       41           
  Lines        2225     2226    +1     
=======================================
+ Hits         2184     2185    +1     
  Misses         41       41           

☔ 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.

@nikosbosse

Copy link
Copy Markdown
Collaborator Author

Automated review (Claude Fable, directed by @nikosbosse):

Verdict: approve. All checks passed; no must-fix findings.

  • Fail-before/pass-after verified independently: on this branch the changed test files pass; with R/ reverted to origin/main (tests kept) the new/corrected expectations fail exactly as claimed — missing ncols error for the 3x4 matrix, and wrong-permutation scores for both logs_categorical() and rps_ordinal() (5 failures total, matching the PR description).
  • Full suite green: 0 failed, 910 passed (R_DATATABLE_NUM_THREADS=2).
  • Fix correctness: order(as.numeric(predicted_label)) is the correct inverse permutation; drop = FALSE preserves the n = 1 path. Independent edge-case checks all correct: n = 1 with vector input and permuted labels (both metrics), NA in observed under permuted labels, 2-level ordinal with reversed labels, unused observed levels, and wrong-width matrix via the ordinal path. The fix's assumption that predicted_label is a full permutation of the levels is guarded by pre-existing validation (len = N, any.missing = FALSE, empty.levels.ok = FALSE, set-equal levels), so duplicated/partial labels are rejected before reordering.
  • Tests are meaningful: the invariance tests use a 3-cycle permutation, which distinguishes the forward from the inverse permutation (a transposition would not); the corrected pre-existing expectation (predicted[, order(c(3, 1, 2))]) is verifiably right, and the nominal test additionally pins -log(p_observed) explicitly.
  • Scope/conventions: diff limited to the two metric files, the two test files, and NEWS; NEWS entry accurate (including the new-error caveat per the maintainer decision); title format correct; no lint issues on changed lines; no roxygen changes needed (documented nxN contract unchanged — the stricter check now enforces it).

@nikosbosse
nikosbosse marked this pull request as draft July 18, 2026 13:53
@nikosbosse

Copy link
Copy Markdown
Collaborator Author

lgtm

@nikosbosse
nikosbosse marked this pull request as ready for review July 18, 2026 20:48
@nikosbosse
nikosbosse requested a review from sbfnk July 18, 2026 20:49
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.

rps_ordinal() and logs_categorical() mishandle permuted predicted_label; missing ncol check for n > 1

1 participant