Skip to content

1198: Fix bias_quantile() quantile level sorting and na.rm crash#1205

Open
nikosbosse wants to merge 1 commit into
mainfrom
issue-1198-bias-quantile-sorting
Open

1198: Fix bias_quantile() quantile level sorting and na.rm crash#1205
nikosbosse wants to merge 1 commit into
mainfrom
issue-1198-bias-quantile-sorting

Conversation

@nikosbosse

Copy link
Copy Markdown
Collaborator

Description

This PR closes #1198.

bias_quantile() had two related bugs in bias_quantile_single_vector() (R/metrics-quantile.R):

  1. Wrong results with unsorted quantile levels: predictions were reordered by quantile level (predicted <- predicted[order]) but quantile_level itself was left unsorted, so predictions and levels became mispaired. The same forecast then produced different bias values depending on input order, e.g. bias_quantile(1.5, c(1, 2, 3), c(0.25, 0.5, 0.75)) returned 0.5 while the identical forecast bias_quantile(1.5, c(3, 1, 2), c(0.75, 0.25, 0.5)) returned -0.5. Fixed by sorting quantile_level together with predicted.

  2. Crash with na.rm = TRUE: when NA removal stripped all quantile levels on one side of the median, interpolate_median() produced a length-zero median (via max(numeric(0)) returning -Inf with a warning) and the function crashed with argument is of length zero. Fixed by returning NA_real_ when the median is no longer interpolable after NA removal, consistent with na.rm = FALSE, which also returns NA_real_ silently.

New regression tests were written first and confirmed to fail on main:

Failure ('test-metrics-quantile.R:817:3'): bias_quantile() is invariant to the order of quantile levels
Expected `bias_quantile(...)` to equal `0.5`. Differences: `actual`: -0.5, `expected`: 0.5

Error ('test-metrics-quantile.R:766:3'): bias_quantile() returns NA when na.rm removes one side of the median
Error: argument is of length zero
Warning: no non-missing arguments to max; returning -Inf

After the fix, the full test suite passes (0 failed, 911 passed). Note one intentional behaviour change beyond bugfixing correctness: internally consistent forecasts passed with unsorted quantile levels, which previously either returned wrong values or errored with "Predictions must not be decreasing", now score correctly; genuinely inconsistent input still errors as before (existing tests unchanged).

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 user-facing documentation changes needed; no roxygen comments were changed.)
  • I have built the package locally and run rebuilt docs using roxygen2. (N/A — no roxygen comments were changed, so no doc rebuild was needed.)
  • My code follows the established coding standards and I have run lintr on the changed files 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.

bias_quantile_single_vector() sorted predictions by quantile level but
left the quantile levels themselves unsorted, so predictions and levels
became mispaired and unsorted input produced silently wrong bias values.
Both vectors are now reordered together.

Additionally, when na.rm = TRUE removed all quantile levels on one side
of the median, interpolate_median() produced a length-zero median and
the function crashed. bias_quantile() now returns NA_real_ in this
case, consistent with na.rm = FALSE.

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 (dfaa4d6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1205   +/-   ##
=======================================
  Coverage   98.15%   98.15%           
=======================================
  Files          41       41           
  Lines        2225     2228    +3     
=======================================
+ Hits         2184     2187    +3     
  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. Independently verified fail-before/pass-after and found no defects.

Checks performed:

  • Fail-before/pass-after confirmed: with R/ reverted to main (tests kept), the new tests fail exactly as claimed — order-invariance test returns -0.5 instead of 0.5, and the na.rm test errors with "argument is of length zero" plus the max() -Inf warning. On the branch, both pass.
  • Full test suite green on the branch (0 failures; 18 environment-dependent skips, mostly vdiffr).
  • Edge cases probed beyond the PR's tests, all correct: a 200-case random permutation grid (unsorted levels bit-identical to sorted); all-NA predictions with na.rm = TRUENA; NA in quantile_level (not predicted) stripping one side → NA; NA at the 0.5 level with both sides surviving → correctly interpolated; unsorted + NA combined; single 0.5-level input; matrix input with unsorted levels; genuinely inconsistent input still errors.
  • Brief compliance: matches maintainer decisions exactly (sort predicted and quantile_level together; silent NA_real_ when NA removal leaves the median non-interpolable, no warning).
  • Scope/conventions: diff touches only R/metrics-quantile.R, the test file, and NEWS.md; NEWS entry accurate; PR title format correct; the intentional error-to-value behaviour change for consistent-but-unsorted input is disclosed in the PR body and I confirmed it behaves as described.

No must-fix findings.

@nikosbosse
nikosbosse marked this pull request as draft July 18, 2026 13:53
@nikosbosse
nikosbosse marked this pull request as ready for review July 18, 2026 20:50
@nikosbosse

Copy link
Copy Markdown
Collaborator Author

lgtm

@nikosbosse
nikosbosse requested a review from seabbs July 18, 2026 20:51
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.

bias_quantile() wrong with unsorted quantile levels and crashes with na.rm = TRUE

1 participant