Skip to content

1211: Fix sample/quantile validation type checks and PIT histogram/rounding messages#1216

Draft
nikosbosse wants to merge 1 commit into
mainfrom
issue-1211-misc-validation-messages
Draft

1211: Fix sample/quantile validation type checks and PIT histogram/rounding messages#1216
nikosbosse wants to merge 1 commit into
mainfrom
issue-1211-misc-validation-messages

Conversation

@nikosbosse

Copy link
Copy Markdown
Collaborator

Description

This PR closes #1211.

Three related validation/messaging bugs, all part of the bug audit in #1189:

  1. Missing type checks: as_forecast_sample(), as_forecast_quantile() and as_forecast_multivariate_sample() accepted non-numeric observed/predicted columns (e.g. character), with the failure only surfacing later inside score() as one warning per metric and an effectively empty scores table. The corresponding assert_forecast() methods now check checkmate::assert_numeric() on both columns, matching the behaviour of the binary/point (and multivariate point) methods, which already errored at validation time.
  2. Wrong rounding message: the quantile-level rounding warning in as_forecast_quantile() claimed round(x, digits = 10) while the code executes round(x, digits = 9). The message now says 9 digits (the code is intentionally unchanged: 10-digit rounding would not collapse diffs of exactly 1e-10, the trigger threshold) and the dangling parenthesis is closed.
  3. get_pit_histogram.forecast_quantile(): the cli_warn() call passed the second sentence as an unnamed second argument, which rlang interprets as the condition class, so users never saw "The PIT histogram will be based on the quantiles present in the forecast." — and that fallback never actually happened: num_bins = 7 returned a 0-row table, and partially-present breaks produced recycling warnings with silently wrong densities. The warning now shows both lines, and the fallback is implemented: requested quantiles present in the forecast are kept; if none besides 0 and 1 remain, all present quantiles are used. diffs is computed after this adjustment so densities, bins and mids stay aligned. The default (num_bins = NULL) path is unchanged.

Failing-before evidence (new tests run against unfixed code):

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

FAILURE: 'test-class-forecast-quantile.R:461:3' ----------
Expected `w` to match regexp "The PIT histogram will be based on the quantiles present in the forecast".
Actual text:
x | Some requested quantiles are missing in the forecast.

FAILURE: 'test-class-forecast-quantile.R:469:3' ----------
Expected `nrow(res)` to be identical to `n_models * (length(present) + 1L)`.
Differences:
  `actual`:   0
`expected`: 120

FAILURE: 'test-class-forecast-quantile.R:484:3' ----------
Expected `w` to have length 1.
Actual length: 5.
(4 extra "longer object length is not a multiple of shorter object length" warnings)

After the fix, the affected test files and the full suite pass (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. (N/A — no user-facing documentation changes; only @importFrom tags were touched and the required imports already exist in NAMESPACE)
  • I have built the package locally and run rebuilt docs using roxygen2. (N/A — re-documenting was skipped deliberately: the locally installed roxygen2 (8.0.0) is newer than the repo's RoxygenNote (7.3.3) and would churn DESCRIPTION and unrelated man/ files; no generated files change from this PR)
  • 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. (CI pending at time of opening; will address any failures)

- assert_forecast() for sample, quantile and multivariate sample forecasts
  now errors when observed or predicted are not numeric, instead of the
  failure only surfacing later as per-metric warnings inside score()
- the rounding warning in as_forecast_quantile() now correctly says
  round(x, digits = 9), matching the code, and closes its parenthesis
- get_pit_histogram.forecast_quantile() now emits both sentences of its
  warning (the second previously landed in the condition class) and
  actually falls back to the quantiles present in the forecast: requested
  quantiles that are present are kept; if none besides 0 and 1 remain, all
  present quantiles are used. Previously the function returned an empty
  data.table (num_bins) or silently recycled wrong densities (breaks).

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.38%. Comparing base (06ed56d) to head (db63afc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
+ Coverage   98.15%   98.38%   +0.23%     
==========================================
  Files          41       41              
  Lines        2225     2235      +10     
==========================================
+ Hits         2184     2199      +15     
+ Misses         41       36       -5     

☔ 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):

Verified:

  • Fail-before/pass-after confirmed independently: with the PR's tests but R/ reverted to main, exactly the 6 new/modified tests fail (14 expectations, for the stated reasons: no error on character observed/predicted, "digits = 10" message, missing second warning line, 0-row num_bins = 7 result, recycling warnings on partial breaks). On the branch all pass.
  • Full suite green: 0 failed, 922 passed.
  • Matches maintainer decisions: numeric checks added to sample/quantile/multivariate-sample assert_forecast() methods (multivariate point already errors via check_input_point(), so correctly untouched); message fixed to 9 digits with code unchanged; cli_warn shows both lines and the hybrid fallback is implemented with diffs computed after adjustment.
  • Own edge cases: integer columns still accepted; factor/logical observed now error cleanly; single present break (breaks = 0.25) works without warning, densities integrate to 1; single missing break falls back to all present quantiles; default num_bins = NULL path unchanged.
  • NEWS entry accurate; @importFrom additions covered by existing NAMESPACE (checkmate::assert_numeric already imported, so skipping roxygen was safe); scope tight (7 files); title format correct.

Minor observations (pre-existing, not introduced by this PR, no action required):

  • Unsorted breaks (e.g. c(0.5, 0.25)) still produce negative densities and garbage bins; since present_quantiles is now sorted, sorting quantiles as well would be a cheap follow-up.
  • num_bins = 1 still silently returns a 0-row table (0 and 1 are always "present", so the new fallback never triggers on that path).

Verdict: approve.

@nikosbosse
nikosbosse marked this pull request as draft July 18, 2026 13:53
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.

Missing type checks in sample/quantile validation and message/behaviour mismatches

1 participant