-
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
Option to cap low winds to 0.5 m/s #1635
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 ✨ |
|
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.
Nice! Think you could add a really simple test under tests/test_indices.py
?
Co-authored-by: Trevor James Smith <[email protected]>
It checks 3 conditions: 1. If the UTCI is similar to the expected value at wind velocity higher than 0.5 m/s (original test) 2. If the UTCI is `NaN` when the wind is lower than 0.5 m/s and `wind_cap_min` is set to `False` 3. If the UTCI is similar to the expected value when the wind velocity is lower than 0.5 m/s and `wind_cap_min` is set to `True` NOTE: Previous commit perfomed redundant tests
I expanded the original UTCI test by trying two wind values (2 and 1 km/h, or 0.556 and 0.278 m/s) with and without the option to cap the minimum wind velocities. Now the test checks:
|
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.
This looks clean. Thanks @profesorpaiche!
xclim/indices/_conversion.py
Outdated
wind_cap_min: bool | ||
If True, low wind velocities are capped to a minimum of 0.5 m/s. This ensures | ||
UTCI calculation for low winds. Default value False. |
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.
Oh, would it be interesting to cite Bröde 2012 here ? Or below in the "references" section ?
In any case, the metadata for the publication has to be added to "docs/references.bib" before being cited here with :cite:t:
or added to the :cite:cts:
directive below.
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.
I think is better to cite Bröde 2012 (and also Bröde 2023 Figure 2) here because the user may want to know a bit more context of this specific option. I will include both in the "docs/reference.bib" file and update the description
Included the article of Bröde 2012 in the description of |
@profesorpaiche Thanks for the contribution! |
Pull Request Checklist:
number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
universal_thermal_climate_index
function to cap low wind to a minimum of 0.5 m/sDoes this PR introduce a breaking change?
Other information:
References for the change: