Skip to content

941: Consolidate score() input preparation into a single helper#1194

Open
nikosbosse wants to merge 4 commits into
mainfrom
issue-941-prepare-forecast-for-scoring
Open

941: Consolidate score() input preparation into a single helper#1194
nikosbosse wants to merge 4 commits into
mainfrom
issue-941-prepare-forecast-for-scoring

Conversation

@nikosbosse

@nikosbosse nikosbosse commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR closes #941.

As discussed in the issue, this consolidates the boilerplate input preparation steps shared by all eight score() methods into a single internal helper, prepare_forecast_for_scoring():

prep <- prepare_forecast_for_scoring(forecast, metrics)
forecast <- prep$forecast
metrics <- prep$metrics
forecast_unit <- prep$forecast_unit  # methods that need it

The helper cleans the forecast (clean_forecast(copy = TRUE, na.omit = TRUE)), determines the forecast unit, validates the metrics and converts to a plain data.table — in exactly the order the methods used before, so behaviour is unchanged (full test suite passes locally).

Beyond removing duplication, the helper locks down a subtle ordering constraint in one audited place: the lazy default metrics = get_metrics(forecast) must be forced while forecast still carries its forecast_* class — there is no get_metrics.default(), so forcing it after the methods rebind forecast to a plain data.table would fail with "no applicable method". Keeping validate_metrics() inside the helper makes this impossible to get wrong when adding future forecast classes. This is documented in the helper's roxygen, including the one subtle semantic note: the default is now evaluated on the original (pre-cleaning) forecast object, which makes no difference for the built-in get_metrics() methods as none of them inspect the data.

Methods that do not need forecast_unit (binary, point, multivariate point) simply don't unpack it.

Tests: a new unit test for prepare_forecast_for_scoring() in test-score.R fails on main (the function does not exist there) and passes on this branch; it also checks that the input forecast is not modified by reference. Additionally, scoring an ordinal forecast turned out not to be exercised anywhere in the suite (surfaced by codecov patch coverage), so a score() test for the ordinal format is added too.

The diff was adversarially reviewed by a multi-agent workflow (finder agents per correctness angle + independent verification of every candidate finding); the confirmed findings (documentation precision, a dangling help-page cross-reference, NEWS accuracy, missing ordinal coverage) are addressed in the later commits.

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 (full test suite passes: 0 failures).
  • I have added or updated unit tests where necessary (helper test fails before this PR and passes afterwards; new ordinal score() test).
  • I have updated the documentation if required.
  • I have built the package locally and run rebuilt docs using roxygen2 (unrelated churn from a newer local roxygen2 version was excluded).
  • My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes (no new lints in changed files).
  • 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 (codecov patch failure fixed by the ordinal test).

🤖 Generated with Claude Code

Add internal prepare_forecast_for_scoring() that performs the input
preparation steps previously duplicated across all eight score()
methods: cleaning the forecast, determining the forecast unit,
validating the metrics and converting to a plain data.table.

Validating the metrics inside the helper also locks down a subtle
ordering constraint: the default `metrics = get_metrics(forecast)`
promise must be forced while `forecast` still carries its forecast
class, i.e. before the rebind to a plain data.table.

Closes #941

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.92%. Comparing base (06ed56d) to head (97db99d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1194      +/-   ##
==========================================
+ Coverage   98.15%   98.92%   +0.76%     
==========================================
  Files          41       41              
  Lines        2225     2231       +6     
==========================================
+ Hits         2184     2207      +23     
+ Misses         41       24      -17     

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

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@nikosbosse nikosbosse changed the title Consolidate score() input preparation into a single helper 941: Consolidate score() input preparation into a single helper Jul 18, 2026
nikosbosse and others added 2 commits July 18, 2026 14:56
- Document precisely why metrics are validated inside
  prepare_forecast_for_scoring() and that the lazy default is evaluated
  on the original (pre-cleaning) forecast object.
- Describe the metrics argument locally instead of inheriting it from
  score(), removing a dangling reference to the "Customising metrics"
  section that only exists on the score() help page.
- Reword the NEWS item: determining the forecast unit was previously
  only done by the methods that need it, not duplicated across all.
- Slim the helper tests down to one focused test and make the
  not-modified-by-reference check a real before/after comparison.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Scoring an ordinal forecast was not exercised anywhere in the test
suite, which codecov surfaced as missing patch coverage on the
refactored score.forecast_ordinal() preamble.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@nikosbosse
nikosbosse requested a review from seabbs July 18, 2026 13:47
@nikosbosse

Copy link
Copy Markdown
Collaborator Author

@seabbs this is a little refactor we discussed a while ago. Back then, you liked the idea

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.

Rethink score() and maybe make it not S3 anymore

1 participant