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

rank_histogram tied ranks #364

Merged
merged 32 commits into from Nov 26, 2021
Merged

rank_histogram tied ranks #364

merged 32 commits into from Nov 26, 2021

Conversation

aaronspring
Copy link
Collaborator

@aaronspring aaronspring commented Nov 23, 2021

Description

image

Closes #335

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • pytest

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 main 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

Hamill, T. M. (2001). Interpretation of Rank Histograms for Verifying Ensemble Forecasts. Monthly Weather Review, 129(3), 550–560. doi: 10/dkkvh3

@aaronspring aaronspring added bug Something isn't working feature request labels Nov 23, 2021
@aaronspring aaronspring self-assigned this Nov 23, 2021
@pep8speaks
Copy link

pep8speaks commented Nov 23, 2021

Hello @aaronspring! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-26 01:01:38 UTC

xskillscore/core/probabilistic.py Outdated Show resolved Hide resolved
xskillscore/tests/test_probabilistic.py Outdated Show resolved Hide resolved
@aaronspring aaronspring marked this pull request as ready for review November 23, 2021 12:30
@aaronspring aaronspring removed the request for review from dougiesquire November 24, 2021 02:11
@aaronspring aaronspring marked this pull request as draft November 24, 2021 02:11
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #364 (df38314) into main (b52cc3f) will decrease coverage by 0.13%.
The diff coverage is 88.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
- Coverage   94.58%   94.45%   -0.14%     
==========================================
  Files          24       24              
  Lines        2661     2688      +27     
==========================================
+ Hits         2517     2539      +22     
- Misses        144      149       +5     
Impacted Files Coverage Δ
xskillscore/core/resampling.py 100.00% <ø> (ø)
xskillscore/core/np_deterministic.py 98.49% <66.66%> (-0.75%) ⬇️
xskillscore/core/probabilistic.py 89.85% <86.36%> (-0.50%) ⬇️
xskillscore/core/np_probabilistic.py 100.00% <100.00%> (ø)
xskillscore/tests/test_probabilistic.py 96.76% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 633b1d3...df38314. Read the comment docs.

@aaronspring aaronspring marked this pull request as ready for review November 24, 2021 23:05
@aaronspring
Copy link
Collaborator Author

#258 handled for rank_histogram

@aaronspring
Copy link
Collaborator Author

@raspstephan interested in reviewing/trying out?

Copy link

@benowen-bom benowen-bom left a comment

Choose a reason for hiding this comment

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

I think this is a good fix to #335. The changes LGTM and I've tested the function out on some sample distributions with tied ranks and the behaviour seems correct.

I have provided a suggestion that might speed things up faster, but overall LGTM.

xskillscore/tests/test_probabilistic.py Outdated Show resolved Hide resolved
xskillscore/core/probabilistic.py Outdated Show resolved Hide resolved
@aaronspring
Copy link
Collaborator Author

thanks for the review @benowen-bom implemented your suggestion and it is great to rely on apply_on_axis which is a loop.

Copy link
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

Looks great to me - thanks @aaronspring and @benowen-bom! One very minor comment

xskillscore/core/probabilistic.py Outdated Show resolved Hide resolved
@aaronspring aaronspring merged commit 7848abd into main Nov 26, 2021
@aaronspring aaronspring deleted the rank_hist_ties branch November 26, 2021 01:09
@raspstephan
Copy link

Awesome, thanks @aaronspring for implementing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xs.rank_histogram does not deal appropriately with tied ranks
5 participants