Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address dist_quantiles() deprecation #634

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

brookslogan
Copy link
Contributor

@brookslogan brookslogan commented Mar 19, 2025

Checklist

Please:

  • Make sure this PR is against "dev", not "main" (unless this is a release
    PR).
  • Request a review from one of the current main reviewers:
    brookslogan, nmdefries.
  • Makes sure to bump the version number in DESCRIPTION. Always increment
    the patch version number (the third number), unless you are making a
    release PR from dev to main, in which case increment the minor version
    number (the second number).
  • [-] Describe changes made in NEWS.md, making sure breaking changes
    (backwards-incompatible changes to the documented interface) are noted.
    Collect the changes under the next release number (e.g. if you are on
    1.7.2, then write your changes under the 1.8 heading).
    • don't think this is worth a NEWS entry.
  • See DEVELOPMENT.md for more information on the development
    process.

Change explanations for reviewer

  • See Deal with dist_quantiles() CHECK issue #631 for context. epipredict updated to use hardhat::quantile_pred() rather than its own dist_quantiles() backed by {distributional}. It has some extra constraints and a bug, and epipredict adds onto the bugs. Current main goal is to avoid CHECK noise in unrelated PRs.
  • Adjusts some formatting utilities that were producing bad error messages along the way.
  • Adjusts existing forecast archive test to use hardhat::quantile_pred(), but also skips it as we need something in Add specialized epix_slide for epi_slide_opt #611 or hardhat/epipredict fix to address current is_locf approach using dplyr::lag with triggers the hardhat+epipredict issue.
  • Adds another forecast archive test which fails even with Add specialized epix_slide for epi_slide_opt #611 and requires the hardhat fix, and thus also skips it for now.

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

@brookslogan brookslogan requested a review from nmdefries March 19, 2025 22:08
@brookslogan
Copy link
Contributor Author

brookslogan commented Mar 19, 2025

@dajmcdon I think I told you forecast archives were working, but now they are broken again. Preparing to report something upstream so hopefully eventually they will become operational again. If you need forecast archives to work, you can probably wrap the predictions in another layer of list; e.g.

preds <- hardhat::quantile_pred(matrix(1:6,2,3), 1:3/4)
vec_chop(preds)
#> [[1]]
#> <quantiles[1]>
#> [1] [3]
#> # Quantile levels: 0.25 0.50 0.75 
#> 
#> [[2]]
#> <quantiles[1]>
#> [1] [4]
#> # Quantile levels: 0.25 0.50 0.75 

@brookslogan brookslogan force-pushed the lcb/address-dist_quantiles-deprecation branch from fbdb39f to 4555ed1 Compare March 19, 2025 22:17
@brookslogan brookslogan force-pushed the lcb/address-dist_quantiles-deprecation branch from 4555ed1 to 02d4d26 Compare March 19, 2025 23:42
@brookslogan
Copy link
Contributor Author

Sorry, think I addressed the CHECK and lint issues and this should hopefully actually be ready now.

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.

Deal with dist_quantiles() CHECK issue
1 participant