-
Notifications
You must be signed in to change notification settings - Fork 59
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
Heat wave magnitude indicator #1926
Heat wave magnitude indicator #1926
Conversation
Welcome, new contributor! It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the
Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨ |
b49d644
to
1aa9c32
Compare
…xclim into heat-wave-magnitude-indicator
This comment was marked as resolved.
This comment was marked as resolved.
…xclim into heat-wave-magnitude-indicator
Co-authored-by: Éric Dupuis <[email protected]>
Co-authored-by: Éric Dupuis <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
@aulemahal I'm now wondering if having the possibility of float input in I think this is really what is meant by a run length of floats, the proper way to generalize. I would not mind keeping the current generalization we have in |
I think we could simply explain the nuance in the docstring ? Like explain what happens when a bool / int / float is passed in the input of either functions. In most cases, I can push such a commit. |
Ok, sounds good. After all it's not like the switch between 1d and nd is much more straightforward elsewhere, e.g. for ufunc_1dim = use_ufunc(ufunc_1dim, da, dim=dim, index=index, freq=freq)
if ufunc_1dim:
rl_stat = statistics_run_ufunc(da, reducer, window, dim)
else:
d = rle(da, dim=dim, index=index)
def get_rl_stat(d):
rl_stat = getattr(d.where(d >= window), reducer)(dim=dim)
rl_stat = xr.where((d.isnull() | (d < window)).all(dim=dim), 0, rl_stat)
return rl_stat
if freq is None:
rl_stat = get_rl_stat(d)
else:
rl_stat = d.resample({dim: freq}).map(get_rl_stat)
return rl_stat |
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.
That looks good to me. Thanks for this nice work!
On a final note, do you have a specific paper in mind on which you base this indicator? I had a discussion on various papers I can't find it now. The indicator in the paper cited in the original issue is a bit different, it has a standardization aspect with a calibration period, so I would not cite this paper, or close the issue yet, the implementation is a bit different to what is done here.
Thank you (and everyone else) also for helping! Happy to contribute 🙌
A colleague cited this paper: https://agupubs.onlinelibrary.wiley.com/doi/full/10.1029/2022EF002833 for the indicator. The paper is about a compound indicator for drought and heat, but the heatwave part follows the definition here (and is in turn referencing the Russo et al., 2014 paper, among others). If I didn't miss anything, there are 2 points in particular about the Russo et al., 2014 definition:
The first point is already captured in the current implementation, because you can calculate the threshold for any reference period and supply the The second point however can not as easily be rebuilt I think. I don't know how much functionality under the Those two do in principle something similar, but the former calculates the threshold over |
Thanks! I just wanted to know which references should be cited, I've added both. I'll merge your PR as soon as the tests have re-passed |
@seblehner Thanks so much for your work on this! xclim v0.53 will be released with your changes either this or next week. Have a good day! |
Closes #994
Pull Request Checklist:
number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
freq
periodDoes this PR introduce a breaking change?
No
TODO: