-
Notifications
You must be signed in to change notification settings - Fork 3
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
add contant baseline model #88
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,117 @@ | |||
if (baseline == "constant baseline") { | |||
# get last observed value for median | |||
|
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.
file name should no include name of model or R. its in the constant-baseline folder so this is repetition.
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.
please update name to be update.R
or similar having constant-baseline
in both folder and filename makes me sad.
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 don't want to make you sad! :(
It is just that all other files are called update-timeseries.R and update-rt.R etc...
|
||
formatted_forecasts <- dplyr::bind_rows(formatted, | ||
point_forecast) %>% | ||
dplyr::relocate(forecast_date, submission_date, target, target_end_date, location, type, quantile, value) |
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.
need to hook into weekly scheduled run time (i.e files in models
)
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.
done - I think
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.
done - I think. Needs checking whether there are any other places this needs to go
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.
needs to be in the aggregated R script in order for the results to be loaded into the submissions
folder.
Thanks @nikosbosse looks good and few stylistic and usage concerns. |
@nikosbosse, what's happening with this one - is the PR ready for another round of review, or are we rethinking adding this at all (@seabbs)? no rush, just checking in. |
So the constant baseline model 'works' - the problem is more with the QRA (that fails with too many models and sometimes runs indefinitely). So I think ideally we need to do the following: look through what is happening in detail, set a maximum to the quantgen runtime, look at the horizon grid we want to have qra for, implement a fallback to equal weights, have a flag somewhere that something goes wrong |
Perhaps drop the NULL model from the QRA and we can instead think about using the NULL model as a stand in if our algorithmic approaches are producing nonsense. @nikosbosse can you make the QRA issues into an issue that someone can then pick up if not able to do in this PR. |
created an issue for QRA: #94 |
I think this can be merged now |
as mentioned in #87