-
Notifications
You must be signed in to change notification settings - Fork 21
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
metrics/probabilities: add quantile score #530
metrics/probabilities: add quantile score #530
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.
Thanks @dplarson. Can you also make a PR to the github.io repo with the general metrics documentation?
Also we'll need a new rc3 whatsnew file for this (we tagged rc2 today ahead of the test trial initialization). |
Sure. I'll finish revising this PR based on your comments and then open a PR to the github.io repo so that we can merge both around the same time. |
4389ac7
to
6746c5e
Compare
Add Quantile Score and Quantile Skill Score for evaluating probabilistic forecasts.
Revise the quantile_score() function use the indicator function form, which, while not as intuitive as explicitly listing out the if-else calculation, is more robust to handling either a single `obs, fx, fx_prob` input or an array of `obs, fx, fx_prob` inputs. But to counteract the non-intuitive nature of the indicator function form, this commit also adds notes in the function docstring to explain how the function behaves for when `obs > fx` and `obs < fx`.
Fix a test that relied on checking the ordering of the probabilistic metric results since the addition of the quantile score and skill score bumped the indexing.
3446675
to
0208c54
Compare
Ignore E501 (line too long) in QS docstring since the specific line is an equation and breaking it up over multiple lines doesn't seem worth it. Ignore W605 (invalid escape sequence '\{' and '\}') in QS docstring since those are necessary strings for the LaTeX equations (i.e. without the escape character, the curly brackets will not be rendered in the readthedocs output).
I think I've now covered all the necessary revisions. Also, the PR for adding the QS and QSS to the website is also ready for review: SolarArbiter/SolarArbiter.github.io#162 |
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.
rtd logs report
ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. If you have updated the package versions, please update the hashes. Otherwise, examine the package contents carefully; someone may have tampered with them.
bokeh==1.4.0 from https://files.pythonhosted.org/packages/de/70/fdd4b186d8570a737372487cc5547aac885a1270626e3ebf03db1808e4ed/bokeh-1.4.0.tar.gz#sha256=c60d38a41a777b8147ee4134e6142cea8026b5eebf48149e370c44689869dce7 (from -r requirements.txt (line 2)):
Expected sha256 c60d38a41a777b8147ee4134e6142cea8026b5eebf48149e370c44689869dce7
Got 8bf87f554dfc00edea05f72324772f15c7ff672a573fcf30cb055aaff89c4eab
hopefully that will magically disappear.
Add additional tests, including for edge cases such as negative infinity.
@wholmgren any other items to review or is this ready to be merged? |
Sorry for overlooking this @dplarson. Can you address this warning? https://github.com/SolarArbiter/solarforecastarbiter-core/pull/530/checks?check_run_id=955308462#step:6:118 Perhaps using the same approach as for the rmse skill score. Also makes me think we should consider a more generic skill score function but let's leave that for another day. |
No problem. I'll address that warning and ping you when ready. And yes, I do agree a more generic skill score function could be beneficial and is worth discussing later. |
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.
Thanks @dplarson. Ok to merge @alorenzo175?
yes |
Add Quantile Score and Quantile Skill Score for evaluating probabilistic
forecasts.
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`
).