Skip to content

1195: Fix assert_forecast() for nominal and ordinal forecasts#1208

Open
nikosbosse wants to merge 1 commit into
mainfrom
issue-1195-nominal-ordinal-assert-forecast
Open

1195: Fix assert_forecast() for nominal and ordinal forecasts#1208
nikosbosse wants to merge 1 commit into
mainfrom
issue-1195-nominal-ordinal-assert-forecast

Conversation

@nikosbosse

Copy link
Copy Markdown
Collaborator

Description

This PR closes #1195.

assert_forecast.forecast_nominal() and assert_forecast.forecast_ordinal() had two bugs:

  1. The incomplete-forecast error named the wrong forecast. The check for complete forecasts filtered complete[(correct), ...] (the forecasts that passed the check) instead of complete[!(correct), ...] when building the error message. As a result the error pointed the user at the first complete forecast, and at NA when every forecast was incomplete. Fixed by inverting the filter in both methods.
  2. The methods visibly returned the forecast object. The documented contract of assert_forecast() is "Returns NULL invisibly", and all other methods (binary, point, quantile, sample) do exactly that; the nominal and ordinal methods ended with return(forecast[]), so a top-level assert_forecast() call auto-printed the entire dataset. Fixed by returning invisible(NULL). No caller consumes the return value, so nothing else changes.

Tests were written first and failed against the unfixed code:

ERROR: 'test-class-forecast-nominal.R:44:3' ----------
Error in `assert_forecast(data)`: ! Found incomplete forecasts
i For a nominal forecast, all possible outcomes must be assigned a probability explicitly.
i Found first missing probabilities in the forecast identified by model == modA
(expected the error to name modB, the incomplete forecast)

FAILURE: 'test-class-forecast-nominal.R:67:3' ----------
Expected `assert_forecast(fc)` to return invisibly.
Actual visibility: visible.

FAILURE: 'test-class-forecast-nominal.R:68:3' ----------
Expected `assert_forecast(fc)` to be NULL.
`actual` is an S3 object of class <forecast_nominal/forecast/data.table/data.frame>, a list

(plus the analogous failures for the ordinal test file). All tests pass after the fix; the full suite passes (0 failed, 916 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 user-facing documentation changes needed; the fix aligns behaviour with the existing docs)
  • I have built the package locally and run rebuilt docs using roxygen2. (N/A — no roxygen comments changed, so no docs rebuild was needed)
  • 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.

- Invert the completeness filter so the incomplete-forecast error
  message names the first incomplete forecast instead of the first
  complete one (or NA when all forecasts were incomplete).
- Return invisible(NULL) as documented and as all other
  assert_forecast() methods do, instead of visibly returning the
  forecast object.

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 (473dba0).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1208   +/-   ##
=======================================
  Coverage   98.15%   98.15%           
=======================================
  Files          41       41           
  Lines        2225     2225           
=======================================
  Hits         2184     2184           
  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 in an isolated checkout:

  • Fail-before/pass-after confirmed. With the PR's tests run against origin/main source: nominal file FAIL 3 (error names modA instead of modB; assert_forecast() returns visibly and non-NULL), analogous for ordinal. On the branch: both files fully green.
  • Full suite green on the branch: 0 failed, 916 passed — matches the PR description.
  • Edge cases probed beyond the PR's tests (all correct after the fix): all-units-incomplete via an unused factor level names the first unit instead of NA; single incomplete unit with a multi-column forecast unit names all unit columns; incomplete unit appearing first in the data is named correctly; as_forecast_nominal() on valid data still returns the forecast object (no caller consumes assert_forecast()'s return value — verified by grep over R/).
  • Scope/conventions: diff limited to the two methods, two test files, and NEWS; NEWS entry accurate and references Nominal/ordinal assert_forecast: incomplete-forecast error names the wrong forecasts and return value is not invisible(NULL) #1195; title follows NNNN: title; docs already promise "Returns NULL invisibly" so no roxygen change needed; new test lines are within the 120-char lint limit.

Minor pre-existing observation (out of scope, no action needed): the ordinal "Printing works as expected" test emits a validation warning because example_ordinal contains NA rows, so the incomplete-forecast branch fires during print; with the corrected filter the message now truthfully names the NA-model group.

@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:07
@nikosbosse

Copy link
Copy Markdown
Collaborator Author

lgtm

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

Nominal/ordinal assert_forecast: incomplete-forecast error names the wrong forecasts and return value is not invisible(NULL)

1 participant