-
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
Elr/rel scores #69
Elr/rel scores #69
Changes from 10 commits
120cd25
4eeb8ed
0234249
1d81151
94d34cc
462adb6
a6f22d1
bec6883
c233d32
4afd00a
4cd6145
36a810f
1718bcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,3 +100,29 @@ error_if_invalid_output_type <- function(output_type) { | |
) | ||
} | ||
} | ||
|
||
|
||
#' Validate relative metrics | ||
#' | ||
#' @noRd | ||
validate_relative_metrics <- function(relative_metrics, metrics, by) { | ||
if (any(is_interval_coverage_metric(relative_metrics))) { | ||
cli::cli_abort( | ||
"Interval coverage metrics are not supported for relative skill scores." | ||
) | ||
} | ||
|
||
if (length(relative_metrics) > 0 && !"model_id" %in% by) { | ||
cli::cli_abort( | ||
"Relative metrics require 'model_id' to be included in {.arg by}." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this strictly necessary? If we know that "model_id' always needs to be included in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't manually checked out the pr and run the code, so this is just from a cursory reading on github - let me know if you'd like me to dig deeper, happy to make a suggestion There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm good with a cursory review on the level of "this is reasonable or not", given that Zhian is also reviewing. Regarding your comment above: I don't think this is strictly necessary, but my general preference is to throw errors guiding users towards what we're expecting rather than modify their inputs. I think all of this is clear to you already, but just to say it:
|
||
) | ||
} | ||
|
||
extra_metrics <- setdiff(relative_metrics, metrics) | ||
if (length(extra_metrics) > 0) { | ||
cli::cli_abort(c( | ||
"Relative metrics must be a subset of the metrics.", | ||
"x" = "The following {.arg relative_metrics} are not in {.arg metrics}: {.val {extra_metrics}}" | ||
)) | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 the alternative to enforcing this requirement is that we add another argument along the lines of
compare
as is used inscoringutils::get_pairwise_comparisons
, setting a default of"model_id"
. I think that would be fine, but since essentially all use cases of this function will include"model_id"
inby
, I don't think it's necessarily worth introducing the extra argument here?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 the current approach is fine since it would be caught by the validation. The problem with extra arguments that affect other arguments is that it becomes difficult for users to remember the relationships between them.