openclimatefix / nowcasting_utils

Common functionality between SatFlow and predict_pv_yield
https://nowcasting-utils.readthedocs.io/en/stable/
MIT License
3 stars 0 forks source link

Issue/66 evaluation #68

Closed peterdudfield closed 3 years ago

peterdudfield commented 3 years ago

Pull Request

Description

Fixes #66

Screenshot 2021-11-30 at 14 55 14 Screenshot 2021-11-30 at 14 55 20

How Has This Been Tested?

Checklist:

review-notebook-app[bot] commented 3 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

peterdudfield commented 3 years ago

This looks great! I like how well-structured the code is: Makes it easy to follow!

For the "absolute" metrics (i.e. the metrics computed using the forecasts and actuals in MW)... I would recommend computing "absolute" metrics for national solar PV. i.e. first add up the MW outturn across all GSPs to get "national" PV generation. Then compute MAE etc. on that "national" figure. (That "national MAE" will almost certainly be the main number that ESO will be interested in!) TBH - and maybe I've missed something - but I'm not sure that computing MAE across GSPs tells a useful story because each GSP has a different capacity? (In contrast, computing the normalised MAE across GSPs definitely makes sense!)

I'm afraid I haven't reviewed the Jupyter Notebooks, BTW (sorry, busy morning!) You've probably already done this but, if not, please do a quick timeseries plot (doesn't need to be well-engineered; just a quick one-off in a notebook) of a few examples, overlaying the forecast and actual PV generation, to check (by eye) that the forecasts and actuals look like they line up correctly (especially important to look for off-by-half-an-hour (because we might be using different conventions for half-hour-ending vs half-hour-beginning) and off-by-an-hour errors (to check for DST problems)).

We cant calculate the national forecast unless we have all the gsp ids. This is perhaps what we should have in the test set, but we can only do that once the test set is expanded a bit. Perhaps once we have done that, we could then add a nationa MAE. Ill make an issue