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

Forecast corrector #68

Merged
merged 19 commits into from
Nov 29, 2020
Merged

Forecast corrector #68

merged 19 commits into from
Nov 29, 2020

Conversation

MRossol
Copy link
Collaborator

@MRossol MRossol commented Nov 25, 2020

@grantbuster Thoughts on data to use for tests?

@MRossol MRossol added the feature New feature or request label Nov 25, 2020
@grantbuster
Copy link
Member

Synthetic data? :)

Copy link
Member

@grantbuster grantbuster 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. Few minor comments. I think tests with synthetic data would be effectve.

Average MEA for all sites
"""
agg_mae = (np.abs(np.nansum(fcsts - actuals, axis=1)).sum()
/ np.nansum(np.nanmax(actuals, axis=0) * 8760))
Copy link
Member

Choose a reason for hiding this comment

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

8760 hard coded? Can't you just use len()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops, took this from Greg, good catch

Returns
-------
agg_mae : float
Aggregate MAE for all sites
Copy link
Member

Choose a reason for hiding this comment

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

What is this? The sum of the MAE for all sites?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I remember yes. Look at the over all MAE for all sites vs the average MAE across sites

fcsts : ndarray
Bias corrected and blended forecasts
"""
fcsts = cls.bias_correct_fcst(actuals, fcsts)
Copy link
Member

Choose a reason for hiding this comment

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

I think you should include in both the method doc strings and the class doc string the exact steps taken to manipulate the forecast data. This will help with transparent documentation propagated through to the docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call

ave_mae = np.nanmean(np.abs(fcsts - actuals).sum(axis=0)
/ (actuals.max(axis=0) * time_steps))
site_mae = np.mean(np.abs(fcsts - actuals), axis=0)
site_rel_mae = site_mae / (np.max(actuals, axis=0) * time_steps)
Copy link
Member

Choose a reason for hiding this comment

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

wait do we actually need to divide by the number of steps anymore?

@MRossol MRossol merged commit 4accfea into master Nov 29, 2020
@MRossol MRossol deleted the fcst branch November 29, 2020 21:36
github-actions bot pushed a commit that referenced this pull request Nov 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants