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

fairBS and fairRPS #211

Merged
merged 20 commits into from
Feb 10, 2021
Merged

fairBS and fairRPS #211

merged 20 commits into from
Feb 10, 2021

Conversation

aaronspring
Copy link
Collaborator

@aaronspring aaronspring commented Oct 13, 2020

Description

add fair brier score https://www-miklip.dkrz.de/about/problems/

  • add as fair=False keyword into brier

  • Same for rps

  • name and cleanup each test in probabilistic (sorry for mixing two PRs into one, the purpose of tests was unclear to me so I gave all tests a docstring and refactored a bit)

  • Question: With fair=True brier_score requires members in forecast, with brier_score(fair=False) it requires probabilities. Should we align this? Or at least allow also member in forecasts for brier_score(fair=False) and do forecast.mean('member') internally (my favorite but not implemented yet)?

Closes #162 #255

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Performance (if you touched existing code run asv to detect performance changes)
  • refactoring

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. This could point to a cell in the updated notebooks. Or a snippet of code with accompanying figures here.

Checklist (while developing)

  • I have added docstrings to all new functions.
  • I have commented my code, particularly in hard-to-understand areas
  • Tests added for pytest, if necessary.

Pre-Merge Checklist (final steps)

  • I have rebased onto master or develop (wherever I am merging) and dealt with any conflicts.
  • I have squashed commits to a reasonable amount, and force-pushed the squashed commits.

References

https://www-miklip.dkrz.de/about/problems/

@aaronspring
Copy link
Collaborator Author

aaronspring commented Jan 29, 2021

source for the tests that the adjustment reduces skill value
image

@aaronspring aaronspring changed the title fairBS fairBS and fairRPS Jan 29, 2021
if weights is not None:
res = res.weighted(weights)
res = xr.apply_ufunc(np.clip, res, 0, 1, dask="allowed") # dirty fix
Copy link
Collaborator Author

@aaronspring aaronspring Jan 31, 2021

Choose a reason for hiding this comment

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

Ideally this shouldn't be needed, I dont understand why without this line I get out-of-bounds results in tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

any thought on this? maybe some weird edge case in my tests @raybellwaves

Copy link
Member

@raybellwaves raybellwaves Feb 10, 2021

Choose a reason for hiding this comment

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

Couple of thoughts. Does it happen on the fair_bool=True tests? If so can move it inside a if fair:

curious is xr.apply_ufunc(np.clip, ... faster that two where statements
a.where(a < 1, 1).where(a > 0, 0) or is it better with dask objects

Copy link
Member

Choose a reason for hiding this comment

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

I see test_rps_limits is a new test so it may have be an issue before this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

still I am not 100% confident with this

xskillscore/core/probabilistic.py Outdated Show resolved Hide resolved
@aaronspring aaronspring removed the request for review from raybellwaves February 1, 2021 11:20
@aaronspring aaronspring marked this pull request as ready for review February 1, 2021 12:07
CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
@raybellwaves
Copy link
Member

Question: With fair=True brier_score requires members in forecast, with brier_score(fair=False) it requires probabilities. Should we align this? Or at least allow also member in forecasts for brier_score(fair=False) and do forecast.mean('member') internally (my favorite but not implemented yet)?

I think doing forecast.mean('member') makes sense. We do that for our example anyway brier_score = xs.brier_score(obs3>.5, (fct3>.5).mean('member'))

Nice work on the tests

@aaronspring aaronspring merged commit 2f9ec3c into xarray-contrib:master Feb 10, 2021
@aaronspring aaronspring deleted the AS_fairBS branch February 10, 2021 16:35
@aaronspring aaronspring mentioned this pull request Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FairBS and FairRPS
2 participants