Closed Bisaloo closed 3 months ago
Nice still digesting but
On an unrelated note, should functions such as sample_to_quantile() be converted to use the class infrastructure? They would become something like as.forecast_sample.forecast_quantile()
I have a PR for you #790
This is great, thanks so much!
I wonder if there should be a forecast abstract class, from which forecast_binary, forecast_point, etc. would inherit. In the docs and exported functions (e.g., is_forecast(), validate_forecast()), things appear as if there is such as class, and the actual class forecast_binary, forecast_point, etc. as not so visible.
Yes I agree.
Summary of action items based on the discussions (in addition to what's mentioned in the general review comments:
purrr::partial
vs. customise_metric()
digits
argument in get_correlations()
across
argument in summarise_scores()
forecast
parent class (we maybe could also do this in 2.1.
- not sure it would break existing code)forecast
object stops becoming valid. @Bisaloo suggested updating [
to achieve that. uhm... I tried splitting up the already accepted changes into a new PR. But when I merged this, github was smart enough to see that the latest commit was the same as this one so this PR got "merged" as well and is closed now. Sorry! I'll split up the open points into different issues
@Bisaloo, you mentioned this:
I was unsure if this should be a short-term or middle-term issue. My main concern with putting it in middle-term is that it will result in breaking changes, which always are a pain, both for users and developers. At the same time, they may require some user feedback, and will likely require some time to implement, so maybe it shouldn’t delay the 2.0.0 release. Some of it has been discussed in the NAMESPACE section but this section mentions other situations as well.
In the same line of thought as https://github.com/epiforecasts/scoringutils/issues/507, it is probably good to have a clearer “happy path” / “recommended workflow”. Options that differ from this path should be removed in many cases to keep the user on the happy path, reduce information overload, and facilitate maintenance. In general, there should not be two nearly identical ways to perform the same task.
For example, in summarize_score(), the by and across arguments are redundant, which is confusing for the user, and forces the developers to jump through extra input checking hoops. Since the transformation of one to the other, I would recommend being more opinionated, and having a single, well-documented option.
I created an issue for across
here: #822. Are there any other specific changes you recommend to make the happy path clearer?
This is a full package review, following the process documented in https://epiverse-trace.github.io/blueprints/code-review.html#full-package-review.
It allows us to use commit suggestions, and annotate the code directly by hijacking GitHub PR review infrastructure.
Once this is done, this PR can be updated to target
main
to directly integrate the changes done it tomain
.