Skip to content

1197: Fix sample metrics for vector input with a single observation#1206

Open
nikosbosse wants to merge 2 commits into
mainfrom
issue-1197-sample-metrics-vector-input
Open

1197: Fix sample metrics for vector input with a single observation#1206
nikosbosse wants to merge 2 commits into
mainfrom
issue-1197-sample-metrics-vector-input

Conversation

@nikosbosse

Copy link
Copy Markdown
Collaborator

Description

This PR closes #1197.

The sample-based metrics document that when observed is a single number, predicted can be given as a vector of samples, and assert_input_sample() accepts this shape. However, four metrics mishandled it:

  • ae_median_sample(1, c(0, 2, 4)) returned 1 1 3 instead of 1 and se_mean_sample(1, c(0, 2, 4)) returned 1 1 9 instead of 1: as.matrix() turned the length-3 vector into a 3x1 matrix (three one-sample forecasts) and the scalar observed was recycled.
  • bias_sample(1, c(0, 2, 4)) errored with 'x' must be an array of at least two dimensions (rowSums()/ncol() on a dimensionless vector).
  • mad_sample(predicted = c(0, 2, 4)) errored with invalid 'times' argument (nrow() of a vector is NULL).

Fix: a new internal helper ensure_sample_matrix() converts a vector of samples into a 1xN matrix after input assertion. It is used in bias_sample(), ae_median_sample(), se_mean_sample() and mad_sample(), and replaces the two existing inline copies of the same idiom in crps_sample() and pit_histogram_sample(). Matrix input is unchanged (results are bit-identical). The PR also corrects the integer bias formula in the bias_sample() docs, which stated P_t(x_t + 1) instead of P_t(x_t - 1) (the code was correct: for a perfect integer forecast the code gives bias 0, while the documented formula would give -1).

Tests were written first and failed against the unfixed code:

FAILURE: 'test-metrics-sample.R:211:3' — Expected `ae` to have length 1. Actual length: 3.
FAILURE: 'test-metrics-sample.R:212:3' — Expected `ae` to equal 1. `actual`: 1 1 3
FAILURE: 'test-metrics-sample.R:244:3' — Expected `se` to equal 1. `actual`: 1 1 9
FAILURE: 'test-metrics-sample.R:257:3' — Expected `mad_sample(predicted = c(0, 2, 4))` not to throw any
  conditions. Actually got a <simpleError>: invalid 'times' argument
ERROR: 'test-metrics-sample.R:275:3' — Error in `rowSums(predicted < observed)`:
  'x' must be an array of at least two dimensions
[ FAIL 13 | WARN 0 | SKIP 0 | PASS 49 ]

After the fix, the full test suite passes (0 failed, 922 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.
  • I have built the package locally and run rebuilt docs using roxygen2.
  • 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.

bias_sample(), ae_median_sample(), se_mean_sample() and mad_sample()
either returned silently wrong results or errored when `observed` was a
scalar and `predicted` a vector of samples, although this input shape is
documented and accepted by assert_input_sample(). All sample metrics now
convert a vector of samples to a 1xN matrix via a new internal helper
ensure_sample_matrix(), which also deduplicates the inline conversions
in crps_sample() and pit_histogram_sample().

Also corrects the integer bias formula in the bias_sample() docs, which
stated P_t(x_t + 1) instead of P_t(x_t - 1) (the code was correct).

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.16%. Comparing base (06ed56d) to head (52f40ca).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1206   +/-   ##
=======================================
  Coverage   98.15%   98.16%           
=======================================
  Files          41       41           
  Lines        2225     2231    +6     
=======================================
+ Hits         2184     2190    +6     
  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: with the new tests kept and R/ reverted to main, exactly the 5 new test blocks fail (13 failures/errors, matching the PR description: wrong length-3 results for ae_median_sample/se_mean_sample, 'x' must be an array for bias_sample, invalid 'times' argument for mad_sample). On the branch the file passes (63 expectations) and the full suite is green (922 passed, 0 failed).
  • Fix correctness: ensure_sample_matrix() is applied only after assert_input_sample(), which restricts vector predicted to the scalar-observed case, so multi-observation input cannot be silently reshaped. Additional edge cases checked by hand and all consistent with the 1-row-matrix equivalent: NA in samples, single-sample vector (length 1), integer-typed input (integer CDF path in bias_sample), empty/character/list predicted (clean checkmate errors preserved, including for mad_sample), crps_sample(separate_results = TRUE) and pit_histogram_sample() refactors are behaviour-neutral.
  • Brief compliance: matches the approved fix plan, including the optional dedup of the two inline conversions and the bias_sample() doc formula correction (P_t(x_t - 1)), with man/ regenerated consistently.
  • Tests: meaningful — length assertions and vector==matrix identity checks would catch plausible wrong fixes (e.g. Nx1 instead of 1xN reshape).
  • Scope/conventions: diff limited to R/metrics-sample.R, tests, NEWS, and the two man pages; NEWS entry accurate and linked to Sample metrics mishandle or crash on documented vector input (scalar observed + vector of samples) #1197; PR title format correct; no lint issues (120-char limit respected).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@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:58
@nikosbosse
nikosbosse requested a review from sbfnk July 18, 2026 20:58
@nikosbosse

Copy link
Copy Markdown
Collaborator Author

lgtm

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.

Sample metrics mishandle or crash on documented vector input (scalar observed + vector of samples)

1 participant