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 GEFS reference forecasts for select networks #240
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.
looks good. a couple of ideas...
@@ -415,6 +424,9 @@ def create_forecasts(api, site, variables): | |||
A site object. | |||
variables : list-like | |||
List of variables to make a new forecast for each of TEMPLATE_FORECASTS | |||
include_probabilistic : boolean, default False |
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 fine. but it seems like we might want to do better at some point. maybe the arg should be templates
instead? then the calling code decides if it wants to pass TEMPLATE_FORECASTS
or TEMPLATE_PROBABILISTIC_FORECASTS
or TEMPLATE_FORECASTS + TEMPLATE_PROBABILISTIC_FORECASTS
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.
looking at the initialize_*
code below and wondering if it would be useful to define TEMPLATE_DETERMINISTIC_FORECASTS
and TEMPLATE_FORECASTS = TEMPLATE_DETERMINISTIC_FORECASTS + TEMPLATE_PROBABILISTIC_FORECASTS
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.
That does also make it easier to reuse the code with other templates, e.g. for trials.
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.
documentation needs updating for new api
docs/source/api.rst
for API changes.docs/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).Enable the creation of reference ProbabilisticForecast objects for MIDC, SOLRAD, SURFRAD, and RTC sites, and scheduled updates using GEFS. API calls to
list_probabilistic_forecasts
should also be a bit faster since we don't really need to get each constant value from the API.Also closes #205.