Improve Growing Season Length and fix rle1d
#349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aulemahal
added
bug
Pull Request Test Coverage Report for Build 1489
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1490
💛 - Coveralls |
huard
approved these changes
Jan 20, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist:
bumpversion (minor / major / patch)has been called on this branchgit push --tags)This PR changes the way
xc.indices.growing_season_lengthis computed. The definition stays the same, but the new method uses two calls toxc.run_length.first_run. The computation can be slower, but the memory use is stable with large datasets and dask. See Ouranosinc/xclim-benchmark#3.Doing so, I also added an input argument to the indicator so that it can run in the southern hemipshere. Supplying
mid_date = '01-01'andfreq='AS-Jul'performs the calculation with a 6 months offset.But here is a question. Right now, we define the start date as the first day with N consecutive days above a threshold, anywhere in the year, but the end date has to happen after the 1st of July. But couldn't there be an edge case where the growing season length starts after the 1st of July?
May be @Paattrriicckk has an opinion?
While implementing the tests, I stumbled upon a bug in
xc.run_length.first_run_ufuncandxc.run_length.windowed_run_*_ufunc. It appears that when getting a dask array,xr.apply_ufuncfirst throws an empty array in. Butxc.run_length.rle1dreturnsNonein that case, which made other numpy operation fail further down. I modified the output for this edge case in a way that fixed the problem.No.