epiforecasts / scoringutils

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

Should the first argument for all functions that take in a `forecast` object be `forecast`? #767

Closed nikosbosse closed 4 months ago

nikosbosse commented 4 months ago

Currently, the first argument is usually called data, e.g. score(data, ...) or get_forecast_counts(data), get_pit(data etc. Maybe it would make sense to call the first argument forecast to make it clear that a forecast object is expected?

The downside is another breaking change, the upside would be potentially more clarity.

@seabbs @sbfnk @jamesmbaazam @toshiakiasakura, what do you think?

toshiakiasakura commented 4 months ago

At first glance, score(forecast, ...) looks forecast argument requires literally some forecasted values or projected values, and not forecast object we want to express. So more explicitly, forecast_object or forecast_data would be preferred but the name is too long...

How about simply object? Then, guide users to check what kind of object is required in the documentation. I agree to separate the argument names for data.frame and forecast objects.

seabbs commented 4 months ago

I think given that the class converter is called as_forecast it is reasonable for users to know that we have forecast objects that we are using (and hence calling the input forecast makes sense to me.

I definitely agree forecast >> data but I have less strong feelings vs other options aside from liking the idea of the name including forecast and wanting it to be short.

nikosbosse commented 4 months ago

I think forecast is nice. It also plays quite nicely into this schema we already have:

image
nikosbosse commented 4 months ago

Done via #772