-
Notifications
You must be signed in to change notification settings - Fork 22
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
Issue #405: expose get_forecast_type()
to users
#466
Conversation
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 all looks fine but we need to fix CI before we can merge this/review it properly
Fix failing CI issues by deleting code remnants that shouldn't be there anymore
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #466 +/- ##
==========================================
Coverage ? 80.93%
==========================================
Files ? 20
Lines ? 1715
Branches ? 0
==========================================
Hits ? 1388
Misses ? 327
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Merged in CI fix - merging #468 into the develop branch should resolve future CI issues |
Should be good for review now :) |
Can you formally ping when ready as it is easier to find vs sorting through all the notifications. |
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.
Looks fine. For safety I would merge in the GH actions fix first. Looks like yet again some random unrelated and undocumented changes in this PR.
@@ -109,3 +109,39 @@ test_that("get_duplicate_forecasts() works as expected for point", { | |||
22 | |||
) | |||
}) | |||
|
|||
|
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.
random space
|
||
|
||
# ============================================================================== | ||
# `get_forecast_type` |
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.
why don't these get functions just have there own test file?
@@ -21,8 +21,6 @@ | |||
correlation <- function(scores, | |||
metrics = NULL, | |||
digits = NULL) { | |||
metrics <- check_metrics(metrics) |
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.
Is this unrelated to this PR?
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.
Yes, but I noted this! 🥺 🐶 It's mentioned in the PR description and you asked me to fix these 🐩
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.
Why is it not its own PR?
Update: Tests are now succeeding apart from R3.5 (see #483) and pkgdown (see #482)
Closes #405.
The function
get_forecast_type()
infers the forecast type based on the input data (a data.frame). This function might potentially be useful to users, so this PRNote: checks were failing due to the issue explained in #468 (i.e. there are two rogue lines of code (
check_metrics()
and a test withscores
instead ofscores_quantile
that make tests fail).