Skip to content
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

ENH: Update convert_units_to function #1206

Merged
merged 16 commits into from Dec 5, 2022
Merged

Conversation

bzah
Copy link
Contributor

@bzah bzah commented Oct 18, 2022

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added
  • The relevant author information has been added to AUTHORS.rst and .zenodo.json

What kind of change does this PR introduce?

This PR adds standard-name-aware unit conversions. Those are listed in data/variables.yml in a new "conversions" section. For now, the amount2rate and the new amount2lwethickness are available (and reverse operations).

convert_units_to will also perform those conversion automatically if the change in dimensionality requires it and the input has a valid standard name.

The new functions are often the equivalent of the "hydro" context. Both are complementary: the hydro context is still to be used in indicators for which the transformation is implicitly valid, no matter the CF attributes.

This is necessary to be able to compute precipitation indices on datasets that express RR as "thickness_of_rainfall_amount" instead of the usual "precipitation_flux" such as the e-obs datasets.

I can't make these transformations available automatically in the indices with the current structure state of declare_units, I'll look into this in a future PR. For now, as shown in the edited units.ipynb these "CF conversions" must be down explicitly beforehand.

Does this PR introduce a breaking change?

Yes, the error pint.errors.DimensionalityError is no longer raised when attempting to convert this kind of datasets described above into a [length] / [time] compatible unit.

Added auto conversion from amount to rate
 when the input variable is known to be a
 convertible amount.
@bzah bzah requested a review from aulemahal October 18, 2022 09:40
[skip ci] No need to run CI for that.
@bzah
Copy link
Contributor Author

bzah commented Oct 20, 2022

A more generalist approach could be to do something like (pseudo-code):

if  is_amount(source) and is_rate(target):
  src_rate = amount2rate(source)
  return convert(src_rate, target)
elif is_rate(source) and is_amount(target):
   src_amount rate2amount(source)
   return convert(src_amount, target)

That would work for "thickness_of_rainfall_amount" case, but also for any attempted conversion between an amount and a rate.

WDYT ?

xclim/core/units.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice thanks!
Apart from my comment on amount2rate, I think this automatic behaviour is right in xclim's scope.

I was going to suggest to open this up to all other thickness/amount<->rate/flux conversions, but I realized that would need extra care because of the non-liquid precipitation where the amount<->rate and thickness<->flux conversions are non-trivial.
If we were to add them, I think the machinery would be more complex and differentiate between all 4 categories and their possible conversions.
(Recall : amount= [kg m-2], thickness=m, rate=[m s-1] and flux=[kg m=2 s=1])

xclim/core/units.py Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
bzah added a commit to cerfacs-globc/icclim that referenced this pull request Oct 20, 2022
This is meant to fix the issue we have with e-obs dataset where
the precipitation is not a flux but rather a "thickness_of_rainfall_amount".
Now, it will be automatically converted to a rate using xclim amount2rate function.
Note that if the dataset is discontinuous, amount2rate might convert,
amounts to a wrong rate[0].

[0] Ouranosinc/xclim#1206 (comment)
@huard huard self-assigned this Nov 8, 2022
@github-actions github-actions bot added the indicators Climate indices and indicators label Nov 8, 2022
@huard
Copy link
Collaborator

huard commented Nov 8, 2022

@aulemahal Ready for final review. Should we add a test for irregular time series to confirm it fails ?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the docs Improvements to documenation label Nov 29, 2022
xclim/core/units.py Outdated Show resolved Hide resolved
@aulemahal aulemahal closed this Nov 29, 2022
@aulemahal
Copy link
Collaborator

Woupsi sorry.

@aulemahal aulemahal reopened this Nov 29, 2022
@aulemahal aulemahal requested a review from huard November 29, 2022 22:21
@aulemahal
Copy link
Collaborator

@huard and the others, I believe this is ready!

I edited the top post to explain what was done here. Grosso modo : Instead of abel's units-based hook into converts_units_to, I chose to go with a metadata-based conversion with it's own functions. convert_units_to can also make use of these. Unit conversion that changes the dimensionality is now possible, but requires valid standard names.

docs/notebooks/units.ipynb Outdated Show resolved Hide resolved
docs/notebooks/units.ipynb Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Nov 30, 2022
aulemahal and others added 2 commits November 30, 2022 13:43
Co-authored-by: David Huard <huard.david@ouranos.ca>
@aulemahal aulemahal merged commit ea5ee50 into master Dec 5, 2022
@aulemahal aulemahal deleted the enh/auto_convert_thickness branch December 5, 2022 15:33
@bzah
Copy link
Contributor Author

bzah commented Dec 5, 2022

Nice to see this being merge 🎉
Thanks for taking over on this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests docs Improvements to documenation indicators Climate indices and indicators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert [length] into [length]/[time] when cell_methods and/or standard_name allows it
4 participants