Skip to content

1201: Error on conflicting observed values within a forecast unit#1207

Open
nikosbosse wants to merge 2 commits into
mainfrom
issue-1201-observed-constancy-check
Open

1201: Error on conflicting observed values within a forecast unit#1207
nikosbosse wants to merge 2 commits into
mainfrom
issue-1201-observed-constancy-check

Conversation

@nikosbosse

Copy link
Copy Markdown
Collaborator

Description

This PR closes #1201.

Forecast validation accepted data in which the observed value differed across rows of the same forecast unit. Such data passed as_forecast_quantile(), as_forecast_sample(), as_forecast_nominal(), as_forecast_ordinal() and as_forecast_multivariate_sample() without complaint, and score() then silently produced multiple wrong score rows for a single forecast (e.g. a quantile forecast with observed = c(10, 10, 20) across three quantile levels validated fine and yielded two wis rows: 7.67 and 17.67). The existing duplicate check groups by c(forecast_unit, type_cols), so rows differing only in observed were never flagged, and the observed = unique(observed) aggregation in the score() methods silently recycled.

The fix adds an internal check, check_observed_constant(), called from assert_forecast_generic() right after the duplicate check, which errors (per maintainer decision: hard error, wording mirroring the duplicate-forecasts message) when a forecast unit has more than one distinct non-NA observed value. Because it lives in the generic validation, as_forecast_*(), assert_forecast(), score() and [.forecast all catch it, for every forecast type. Rows with NA observed are ignored, and multivariate forecasts need no special-casing: the forecast unit is the univariate unit, within which observed is legitimately constant (a regression test using example_sample_continuous with joint_across = c("location", "location_name") guards this).

Tests were written first and failed against the unfixed code:

FAILURE: 'test-class-forecast.R:422:3' ------------
Expected `as_forecast_quantile(...)` to throw a error.

FAILURE: 'test-class-forecast.R:433:3' ------------
Expected `as_forecast_sample(...)` to throw a error.

FAILURE: 'test-class-forecast.R:444:3' ------------
Expected `as_forecast_nominal(...)` to throw a error.

After the fix, the full test suite passes (0 failed, 915 passed).

Note: this is a user-facing behaviour change — previously accepted (but silently mis-scored) data now errors at validation. A NEWS.md entry documents this.

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() 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. (N/A at time of opening — will monitor CI once it runs.)

…st unit (#1201)

Add an internal check, check_observed_constant(), called from
assert_forecast_generic(), that errors when rows belonging to the same
forecast unit have different observed values. Previously such invalid
data passed validation for quantile, sample, nominal, ordinal and
multivariate sample forecasts and score() silently returned multiple
wrong score rows for a single forecast. Rows with NA observed values
are ignored by the check.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1207      +/-   ##
==========================================
+ Coverage   98.15%   98.16%   +0.01%     
==========================================
  Files          41       41              
  Lines        2225     2239      +14     
==========================================
+ Hits         2184     2198      +14     
  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. Verified in an isolated worktree at the PR head commit.

  • Fail-before/pass-after confirmed: with R/ reverted to main (tests kept), the new tests fail exactly as claimed (3 failures on the as_forecast_* expect_error tests, 2 errors from the missing helper); on the branch the file passes (59/59).
  • Full suite green: 915 passed, 0 failed, 0 errors — matches the PR description.
  • Edge cases probed beyond the PR's tests (all correct): ordinal forecasts with conflicting observed error; a unit whose observed is entirely NA still validates; observed = c(5, NA, 7) errors; score() on a pre-existing invalid object errors via clean_forecast(); point forecasts keep the (better) duplicate-forecast message because the constancy check runs after check_duplicates(); data with a minimal/empty forecast unit is handled correctly (whole table treated as one unit).
  • Matches the maintainer decision (hard error, wording mirrors the duplicates message), scope is minimal, NEWS entry accurate, docs regenerated, title format correct, lintr clean (120-char limit).

Minor, non-blocking:

  1. Ordinal has no automated test (nominal covers the shared code path; I verified ordinal manually). A one-liner would future-proof it.
  2. Trivial: the new helper adds an extra consecutive blank line before the "Clean forecast object" section.

@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 21:04
@nikosbosse

Copy link
Copy Markdown
Collaborator Author

lgtm

@nikosbosse
nikosbosse requested a review from seabbs July 18, 2026 21:04
- Add an automated test that as_forecast_ordinal() errors on
  conflicting observed values within a forecast unit
- Remove extra blank line after check_observed_constant()

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

Copy link
Copy Markdown
Collaborator Author

Follow-up (Claude Fable, directed by @nikosbosse): addressed both minor review findings in a11159e.

  1. Ordinal test added: tests/testthat/test-class-forecast-ordinal.R now has an automated test that as_forecast_ordinal() errors with the new "different observed values" message when a forecast unit has conflicting observed values (ordered-factor data, matching the style of the nominal test added by this PR).
  2. Whitespace: reduced the three consecutive blank lines after check_observed_constant() in R/class-forecast.R to the file's usual two.

Verification: devtools::test(filter="class-forecast") passes (284 tests, 0 failures) and the full suite is green (0 failed, 919 passed). lintr reports no lints on the changed test file and nothing new on R/class-forecast.R.

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.

Validation permits conflicting observed values within a forecast unit

1 participant