Skip to content

1196: Fix as_forecast_quantile() from samples for asymmetric probs#1218

Draft
nikosbosse wants to merge 2 commits into
mainfrom
issue-1196-quantile-from-sample-asymmetric-probs
Draft

1196: Fix as_forecast_quantile() from samples for asymmetric probs#1218
nikosbosse wants to merge 2 commits into
mainfrom
issue-1196-quantile-from-sample-asymmetric-probs

Conversation

@nikosbosse

Copy link
Copy Markdown
Collaborator

Description

This PR closes #1196.

as_forecast_quantile.forecast_sample() built the quantile_level column from a symmetrised vector unique(round(c(probs, 1 - probs), 10)) while computing the actual quantiles at the raw probs — a refactoring regression from 4d9ecf7 (#876), where the predecessor sample_to_quantile() used the same symmetrised vector for both. As a result, any probs not symmetric around 0.5 either silently paired quantile values with the wrong levels (via data.table length-1 recycling) or errored, and duplicated probs triggered a duplicate-forecast assertion.

The fix (per maintainer decision, Option B): compute quantiles at exactly the requested probs, using a single quantile_level <- unique(round(probs, digits = 10)) for both the column and the quantile() call — matching the documented behaviour of probs. For every symmetric prob set, including the default c(0.05, 0.25, 0.5, 0.75, 0.95), the output is byte-identical to before. The input check is also tightened to assert_numeric(probs, min.len = 1, lower = 0, upper = 1) for a clearer early error on out-of-range probs.

New tests failed as expected before the fix:

FAILURE: probs = 0.4 ->
  `actual`: 40.6 40.6      # two rows; level 0.6 silently got the 0.4 quantile
`expected`: 40.6

FAILURE: probs = c(0.1, 0.2) ->
  Error: Supplied 2 items for column 2 of group 1 which has 4 rows. ...

ERROR: probs = c(0.5, 0.5) ->
  Error: Assertion on 'data' failed: There are instances with more than one
  forecast for the same target. ...

All tests pass after the fix; the full suite passes on main + this change (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 roxygen changes; the fix makes the code match the existing documentation of probs).
  • I have built the package locally and run rebuilt docs using roxygen2 (N/A — no roxygen comments changed, man/ files unaffected).
  • My code follows the established coding standards and I have run lintr on the changed files (no new lints introduced).
  • 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.

as_forecast_quantile.forecast_sample() built the quantile_level column
from a symmetrised vector unique(round(c(probs, 1 - probs), 10)) while
computing the quantiles at the raw probs, a refactoring regression from
4d9ecf7 (#876). For asymmetric probs this silently paired quantile
values with the wrong levels (via length-1 recycling) or errored; for
duplicated probs it triggered a duplicate-forecast assertion.

Quantiles are now computed at exactly the requested (deduplicated)
probs, matching the documentation. Symmetric prob sets, including the
default, produce byte-identical output to before. probs is now also
bounded to [0, 1] for a clearer early error.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1218      +/-   ##
==========================================
- Coverage   98.15%   98.15%   -0.01%     
==========================================
  Files          41       41              
  Lines        2225     2224       -1     
==========================================
- Hits         2184     2183       -1     
  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.

Checks performed (in an isolated detached worktree at e56c754):

  • Fail-before/pass-after verified: with R/ reverted to origin/main (tests kept), all 4 new tests fail for the stated reasons (recycled 0.4 quantile onto level 0.6; data.table RHS-length error for c(0.1, 0.2); duplicate-forecast assertion for c(0.5, 0.5); quantile()'s internal error instead of the checkmate assertion for out-of-range probs). With the branch source restored, all pass.
  • Full suite green: 872 passed, 0 failed, 18 skipped.
  • Own edge-case probes (beyond the PR's tests): unsorted probs c(0.75, 0.2) across two forecast units pair levels and values correctly; boundary probs c(0, 1) return min/max; near-duplicate probs beyond the 1e-10 rounding precision dedupe to one row; default probs output is byte-identical to main, confirming no behaviour change for symmetric prob sets.
  • Brief compliance: implements Option B (quantiles at exactly the requested probs, no symmetrization) and the approved lower = 0, upper = 1 bounds, as decided. Roxygen for probs already describes exactly the new behaviour, so no doc changes needed. Scope is minimal (source fix + tests + NEWS), NEWS entry accurate, PR title follows the NNNN: title convention, checklist truthful.

One minor observation (not blocking, pre-existing on main): probs = c(0.5, NA) passes assert_numeric() (bounds ignore NAs) and silently yields a row with quantile_level = NA / predicted = NA. Identical behaviour before and after this PR; any.missing = FALSE could be added in a follow-up if desired.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@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.

as_forecast_quantile.forecast_sample() broken for asymmetric probs

1 participant