Skip to content
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

Reference forecast limit #727

Merged
merged 11 commits into from
Sep 15, 2021

Conversation

lboeman
Copy link
Member

@lboeman lboeman commented Sep 11, 2021

  • Closes Limit range of persistence forecast  #721 .
  • I am familiar with the contributing guidelines.
  • Tests added.
  • Updates entries to docs/source/api.rst for API changes.
  • Adds descriptions to appropriate "what's new" file in 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

@lboeman lboeman added the enhancement New feature or request label Sep 13, 2021
@lboeman lboeman added this to the 1.0.5 milestone Sep 13, 2021
Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall

solarforecastarbiter/reference_forecasts/utils.py Outdated Show resolved Hide resolved

def _limit_persistence_run_time(data_start, max_run_time, forecast):
"""Get a the last run time that would result in the
forecast producing points as limited by PERS_PT_LIMIT.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's PERS_PT_LIMIT, but PERSISTENCE_PT_LIMIT in code and other documentation. I might spell out POINT too.

Don't we prefix some Arbiter env vars with SFA? Not sure how consistent we might be about that so don't know if this is really an issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace doc string with:

Get the last run time.

Last run time is either max_run_time or the time that limits the forecast length to value set by SFA_PERISTENCE_POINT_LIMIT environment variable or DEFAULT_PERSISTENCE_POINT_LIMIT.


pts_per_run = forecast.run_length / forecast.interval_length

max_runs = fx_pt_limit / pts_per_run
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to floor this? max_runs suggests an integer.

generated to the the limit provided by the PERISTENCE_PT_LIMIT env var
or DEFAULT_PERS_PT_LIMIT.
"""
fx_pt_limit = float(os.getenv('PERSISTENCE_PT_LIMIT', DEFAULT_PERS_PT_LIMIT))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

presumably an integer

solarforecastarbiter/reference_forecasts/utils.py Outdated Show resolved Hide resolved
Comment on lines 384 to 385
Timeseries creatd are further limited by the maximum number of points
generated for any one forecast. The default limit is 136800 points,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear. "Each forecast is further limited to a maximum number of points."?

forecast_hr_begin, start, limit, expected, monkeypatch
):
max_run_time = pd.Timestamp('2021-01-10T00:00Z')
monkeypatch.setenv("PERS_PT_LIMIT", str(limit))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually work? Doesn't match the env var in the function

lboeman and others added 3 commits September 14, 2021 15:54
solarforecastarbiter/reference_forecasts/main.py Outdated Show resolved Hide resolved

def _limit_persistence_run_time(data_start, max_run_time, forecast):
"""Get a the last run time that would result in the
forecast producing points as limited by PERS_PT_LIMIT.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace doc string with:

Get the last run time.

Last run time is either max_run_time or the time that limits the forecast length to value set by SFA_PERISTENCE_POINT_LIMIT environment variable or DEFAULT_PERSISTENCE_POINT_LIMIT.

solarforecastarbiter/reference_forecasts/utils.py Outdated Show resolved Hide resolved
lboeman and others added 2 commits September 15, 2021 11:12
Co-authored-by: Will Holmgren <william.holmgren@gmail.com>
@wholmgren wholmgren merged commit da3f04b into SolarArbiter:master Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit range of persistence forecast
2 participants