-
Notifications
You must be signed in to change notification settings - Fork 0
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
Score model out improvements #54
Conversation
@@ -72,7 +78,11 @@ | |||
#' ) | |||
#' head(pmf_scores) | |||
#' | |||
#' @return forecast_quantile | |||
#' @return A data.table with scores |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it's an object of class scores
which is itself a data.table
but probably easiest not to confuse users with scoringutils
stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much better descriptor. Thank you for catching it, especially since data.table
can catch people off-guard since it doesn't behave like other R objects (i.e. modifying a data table will modify the object in-place as opposed to making a copy).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good. I am all in favor of shunting the majority of the work and documentation to scoringutils.
I think we can live with the errors. Replacing them reliably in the future is so not fun. If anything, we might be able to catch the error and prepend it with "oh hey, something in scoringutils went wrong and here's the error message:"
@@ -72,7 +78,11 @@ | |||
#' ) | |||
#' head(pmf_scores) | |||
#' | |||
#' @return forecast_quantile | |||
#' @return A data.table with scores |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much better descriptor. Thank you for catching it, especially since data.table
can catch people off-guard since it doesn't behave like other R objects (i.e. modifying a data table will modify the object in-place as opposed to making a copy).
Co-authored-by: Zhian N. Kamvar <[email protected]>
Co-authored-by: Zhian N. Kamvar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me too, thanks for putting them together! I agree that better error messages would be nice, but I think the errors that people are most likely to encounter in practice are clear.
(builds on #52)
This PR simplifies
score_model_out
, mostly by offloading a bunch of the heavy work toscoringutils
. In particular ithubEvals
toscoringutils
.interval_coverage_XY
a bit to allow for all numbers in (0, 100).mean
andmedian
forecasts to the ones suggested by Gneiting. Personally I think it makes sense to simply not return something we know is flawed.Moving the logic to
scoringutils
makes the code easier and more concise. Errors are handled byscoringutils
which comes with both upsides and downsides. The upside is it's all cleanly handled and we don't need to worry about it.The major downside is that error messages are slightly less "nice". E.g. when you pass something like
metrics = list("a", "b")
toscore_model_out
, then you'll get an error messageAssertion on 'c(select, exclude)' failed: Must be of type 'character' (or 'NULL'), not 'list'.
If we don't like the current error messages I suggest we catch the errors and replace them with new messages. But I think it's still cleaner to have the logic be handled byscoringutils
instead of re-implementing it here.