Exportable function for fao_allen98 #2067
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
|
@saschahofmann There's a configuration in |
aulemahal
reviewed
Feb 5, 2025
aulemahal
reviewed
Feb 5, 2025
coxipi
reviewed
Feb 5, 2025
aulemahal
reviewed
Feb 5, 2025
src/xclim/indices/_conversion.py
Outdated
Comment on lines
1322
to
1332
| # TODO: How can I check exact units for inputs | ||
| # @declare_units( | ||
| # net_radiation="[MJ m-2 day-1]", | ||
| # tas="[degC]", | ||
| # wind="[m s-1]", | ||
| # es="[kPa]", | ||
| # ea="[kPa]", | ||
| # delta_svp="[kPa degC-1]", | ||
| # gamma="[kPa degC]", | ||
| # G="[MJ m-2 day-1]", | ||
| # ) |
There was a problem hiding this comment.
Indeed, the declare_units really is needed only if there's a plan to make this function an indicator itself.
coxipi
reviewed
Feb 5, 2025
coxipi
reviewed
Feb 6, 2025
aulemahal
approved these changes
Feb 6, 2025
I was coming to say so :P |
Co-authored-by: Pascal Bourgault <[email protected]>
|
@saschahofmann Sorry I should have asked if your PR was ready, I hope it was. Since I saw we both accepted, I thought it was ready to go, but that excluded you. I'm not used to be in the position where I do the review and do the merge |
|
All 👍 thanks for merging |
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:
number) and pull request (:pull:number) has been addedWhat kind of change does this PR introduce?
fao_allen98that implements the FAO-56 Penman-Monteith equation and is used insidepotential_evapotranspirationfor theallen98method. This allows providing some variables directly instead of using estimates e.g. tas mean or the different vapour pressures can be taken from datasets directly instead of being estimated. See discussion [Questions] potential_evapotranspiration with FAO providing net radiation directly #1853 or issue Adding pure FAO Penman-Monteith equation to compute potential evapotranspiration #2004.%lead to wrong results for PET when usingallen98Does this PR introduce a breaking change?
No.
Questions:
I would like to fix the units of the input for the new function exactly. Is there an easy way to do this? And how can I provide a scalar value as a default with units?