-
Notifications
You must be signed in to change notification settings - Fork 54
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
Fix wet spells - generic spell statistics #1838
Conversation
Very nice to have this new generic function! |
Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
for more information, see https://pre-commit.ci
Sorry for the unrelated addition. Last commit fixes an issue I had in |
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.
Nicely done!
Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
<!--Please ensure the PR fulfills the following requirements! --> <!-- If this is your first PR, make sure to add your details to the AUTHORS.rst! --> ### Pull Request Checklist: - [ ] This PR addresses an already opened issue (for bug fixes / features) - This PR fixes #xyz - [x] Tests for the changes have been added (for bug fixes / features) - [x] (If applicable) Documentation has been added / updated (for bug fixes / features) - [x] CHANGELOG.rst has been updated (with summary of main changes) - [x] Link to issue (:issue:`number`) and pull request (:pull:`number`) has been added ### What kind of change does this PR introduce? * Implements `generic.season`, a generic index function for computing the start/end/length of seasons. Based on `run_length.season` but with units management and the comparison. * Divide `run_length.season` into `season_start`, `season_end`, plus the existing `season_length`. The idea of grouping code together was good, but it added a lot of unnecessary computations when only one facet is needed (i.e. most cases). I think I was able to divide it elegantly, using only one magic fast path. * Rewrite `frost_free_season_start` and `frost_free_season_end`. Those were not respecting the "season" definition. Now they do. ### Does this PR introduce a breaking change? `frost_free_season_start` and `frost_free_season_end` have changed. Their notion of "season" is not more strict. I think the difference not significative for most cases. No tests needed a change, but again our tests are very simple. All season length indicators will now return 0 when no season is found (no start). Previously, when no start was found we returned 0 when an end was found and `nan` otherwise. I think the idea was to assume that neither end nor start meant the data was probably wrong. But that's the job of the missing checks, not the `run_length` helper. Thus I applied the more logical rule : no season == 0 length. I have reverted some changes of #1796, I'm sorry. I'm trying to generalize the indicators and it made more sense that all aspects of the same "growing season" were using the same exact arguments. ### Other information: This allows indices to use run length function that return a DataArray after being called with a DataArray. This "type preservation" will be quite handy in my next PR, stay tuned! Sadly, this branch is based off #1838 because I need both for PC. ### Ugh The amount of copied docstring text and signatures is ridiculous. I really want to remove all those one-liner indices and only keep the indicators. But I fear this would be too breaking of a change as it would remove elements from the API. Right now, we have inconsistent indicators with inconsistent documentation because everything was copy-pasted by hand with incremental changes that were not always ported back to the other indices.
Pull Request Checklist:
number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
reducer='count'
case in 1drle_statistics
.Does this PR introduce a breaking change?
Yes, results for all wet spells indicators will change. But they were not correct before.
Results for
dry_spell_frequency
are also different, were also not correct previsouly.Other information: