-
Notifications
You must be signed in to change notification settings - Fork 44
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
Use pint for units #160
Use pint for units #160
Conversation
Installs and uses |
It seems like this actually works quite well. The only thing we need now is to ignore models that provide no unit attrs (there are a few 😭). I feel that should be an upstream option though? |
cmip6_preprocessing/preprocessing.py
Outdated
@@ -436,6 +422,8 @@ def combined_preprocessing(ds): | |||
ds = broadcast_lonlat(ds) | |||
# shift all lons to consistent 0-360 | |||
ds = correct_lon(ds) | |||
# codify units with pint | |||
ds = ds.pint.quantify() |
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.
Should we do this inside correct_units
?
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 would fix your bug, but whether or not you want to conceptually combine "adding codified physical units to the data" and "potentially change what units the data is expressed in" is up to you.
So now that I have put in another if statement to not run on the datasets that are missing units it complains about this:
Full error here. I first thought that from pint import UnitRegistry
ureg = UnitRegistry()
'centimeters' in ureg and got not sure what is going on here. |
it is a bit difficult to see, but the |
But we are calling |
that does not seem to be the case for the traceback posted in #160 (comment)? |
Right I see what you mean - in the failing test it only imports |
Thanks a lot for the pointers. I think I get it now (still very new to the whole pint thing). Ill fiddle some more... |
just checking: if you want to get the string |
Ah thanks a lot for checking in @keewis, I misunderstood that one. Lets see if the tests pass this time. Ill try to get this merged today. |
Co-authored-by: keewis <[email protected]>
Co-authored-by: keewis <[email protected]>
Co-authored-by: keewis <[email protected]>
Thanks a lot @keewis and @TomNicholas for getting this started and all the detailed pointers. |
Uses pint-xarray to first add units to the dataset during pre-processing, then convert depth to metres. Might help close #31
Doesn't seem to break any tests (I had one fail locally but that also failed before I made any changes).
Not really sure what other changes might be required to use pint in this package properly but this is a start.