epiforecasts / scoringutils

Utilities for Scoring and Assessing Predictions
https://epiforecasts.io/scoringutils/
Other
47 stars 19 forks source link

Issue #817 - Create forceast super class #813

Closed nikosbosse closed 4 weeks ago

nikosbosse commented 1 month ago

Description

This PR closes #817.

As discussed in #791, this PR adds an additional forecast super class and replaces the current print method with it. I also slightly updated an error message. In addition, the PR udpates docs, the NEWS file, and the manuscript.

Checklist

nikosbosse commented 1 month ago

@seabbs the PR is still missing documentation. I requested a review from you to make sure this is what we want. @Bisaloo I'd also be happy to hear your thoughts if you like to chime in.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.52%. Comparing base (b58434e) to head (f29dd7b).

:exclamation: Current head f29dd7b differs from pull request most recent head d677be2

Please upload reports for the commit d677be2 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #813 +/- ## ========================================== - Coverage 95.53% 95.52% -0.02% ========================================== Files 21 21 Lines 1569 1563 -6 ========================================== - Hits 1499 1493 -6 Misses 70 70 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nikosbosse commented 1 month ago

I think yes but you would also expect to see (or plan for each current class to have their own as_forecast_sample() method etc.

But that would be unrelated to this PR, right?

nikosbosse commented 1 month ago

Latest commits update the docs/news/manuscript. This should be ready for review now.

nikosbosse commented 1 month ago

However, do we have an issue for writing explicit as_forecast_quantile etc methods as these seem like a better entry point (these would also add the forecast superclass). I would also personally then vote for getting rid of as_forecast (and lose the automation of guessing the forecast typee but with the benefit of safety and clarity IMO)

Hm that's a good point. Oh maaaan, we're never going to get a CRAN release out the door 😆 🥲 (or we're willing to deprecate as_forecast() at some point)

nikosbosse commented 1 month ago

I'll merge this PR once CI decides it's ready to cooperate with us again. That 3.6 check has been failing quite a lot lately for inscrutable reasons...

Bisaloo commented 1 month ago

CI is failing because scoringutils has an recursive dependency on R 4.0.0 now via evaluate.

pak::pkg_deps_explain("scoringutils", "evaluate")
#> ℹ Loading metadata database
#> ✔ Loading metadata database ... done
#> 
#> scoringutils -> scoringRules -> knitr -> evaluate

Created on 2024-06-12 with reprex v2.1.0

nikosbosse commented 1 month ago

CI is failing because scoringutils has an recursive dependency on R 4.0.0 now via evaluate.

oh.. thanks! Do you have opinions on whether we should

advantage of the second one is that we could start using |>. Disadvantage is of course that some people might not be able to upgrade R....

Bisaloo commented 1 month ago

The tidyverse policy is to depend on R 4.0.0 from now on so I think most people will have to upgrade anyways and there is a low risk to require it in scoringutils. I'd make the switch to R 4.0.0 officially here

nikosbosse commented 1 month ago

We now have

nikosbosse commented 1 month ago

Good to merge?