SolarArbiter / solarforecastarbiter-core

Core data gathering, validation, processing, and reporting package for the Solar Forecast Arbiter
https://solarforecastarbiter-core.readthedocs.io
MIT License
33 stars 21 forks source link

Metrics: make CRPS handle obs outside the fx support #781

Closed dplarson closed 2 years ago

dplarson commented 2 years ago

This PR addresses the issue identified in #779 regarding how the current implementation of the CRPS metric handles cases where the observation is outside the forecast support. For example, if the probabilistic forecasts predicts power values from 1 to 8 MW but the actual power is 10 MW. The core idea is to extend the forecast CDF so covers the observation, whether the observation is less than the minimum forecast (obs < min fx) or greater than the maximum forecast (obs > max fx).

This PR also updates the CRPS function docstring with improved equation rendering and notes on practical considerations related to calculating CRPS from discrete forecast CDFs.

Lastly, the CRPS calculation has been updated to use trapezoidal numerical integration (instead of rectangular integration). This change does not increase the computational cost of the CRPS calculation, but has lower error (O(\delta x) for rectangular vs O(\delta x^2) for trapezoidal, where \delta x is the spacing between points). In practice, this means the CRPS calculation is now more accurate in cases with lower resolution forecast CDFs (e.g., 0%, 10%, ..., 100%).

dplarson commented 2 years ago

CI failed because I forgot about the metrics.calculator tests... Working to fix those.

dplarson commented 2 years ago

@lboeman this PR (for revising the CRPS metric calculation) is ready for review.

lboeman commented 2 years ago

@lboeman let's give @dplarson a chance to address the comments and unless David says otherwise then go ahead and merge - I don't feel a need to look again.

Sounds good. Everything looks good to me. From what I can follow tests align well with the discussion of expected behavior. Thanks for taking care of this so quickly @dplarson.

dplarson commented 2 years ago

Thanks @wholmgren for your fast review. I've revised per your comments.

dplarson commented 2 years ago

@lboeman FYI I'm available the rest of the day if any other changes are needed before merging this PR.

lboeman commented 2 years ago

@lboeman FYI I'm available the rest of the day if any other changes are needed before merging this PR.

I'll go ahead and merge and poke it in dev a bit! Thanks David.