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

Add support for met_em files #46

Merged
merged 18 commits into from
Feb 16, 2022

Conversation

lpilz
Copy link
Collaborator

@lpilz lpilz commented Feb 3, 2022

Change Summary

We already have a discussion about adding support for geo_em files and I wanted to extend this to metgrid met_em files in general. I have identified a number of specific met_em problems and wanted to get some input on how to approach them.

My approach was to call .metpy.quantify() on all variables in a met_em dataset I have and work through the problems sequentially. I finally managed to identify some issues with the variables which I wanted to present und discuss here.

Variable units attr Proposed fix
VAR_SSO meters2 MSL (it's a variance) overwrite units and description in config
HGT_M meter MSL overwrite units and description in config
SM100255 fraction add upstream?
OL4 whoknows overwrite in config?
SCB_DOM category add upstream?
SOILTEMP Kelvin should be working since pint-xarray/issues#26/cf-xarray/pull#197?
COSALPHA_V none add upstream?
SEAICE 0/1 Flag add to _BOOLEAN_UNITS_ATTRS
LANDSEA 0/1 Flag add to _BOOLEAN_UNITS_ATTRS

Maybe @jthielen can say more to the case (in)-sensitivity but as I understood the issue it should already be upstream??

There are some units missing (like for PRES and ST) but I wanted to discuss how to proceed before simply adding those.

Related issue number

Towards #36

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable

@jthielen
Copy link
Collaborator

jthielen commented Feb 5, 2022

I think the approach should be to bring these attributes into CF compliance. So, based on using cf_units (a python wrapper of UDUNITS-2, which defines CF compliance for units), all the units except for "Kelvin" in the table above are invalid and should be fixed (or removed if it's actually dimensionless or without real units).

from cf_units import Unit
unit_list = ["meters2 MSL", "meter MSL", "fraction", "whoknows", "category", "Kelvin", "none", "0/1 Flag"]
for unit_str in unit_list:
    try:
        Unit(unit_str)
        print(f"VALID: {unit_str}")
    except:
        print(f"INVALID: {unit_str}")
INVALID: meters2 MSL
INVALID: meter MSL
INVALID: fraction
INVALID: whoknows
INVALID: category
VALID: Kelvin
INVALID: none
ut_scale(): NULL factor argument
ut_are_convertible(): NULL unit argument
ut_divide(): NULL argument
ut_are_convertible(): NULL unit argument
INVALID: 0/1 Flag

Maybe @jthielen can say more to the case (in)-sensitivity but as I understood the issue it should already be upstream??

Yep, CF-appropriate case handling is indeed an upstream issue in MetPy (Unidata/MetPy#1362), and hopefully those same improvements will be brought over to cf-xarray when they eventually happen!

There are some units missing (like for PRES and ST) but I wanted to discuss how to proceed before simply adding those.

I say go ahead and just add them if they're missing!

@lpilz lpilz marked this pull request as ready for review February 14, 2022 15:36
xwrf/tutorial.py Outdated Show resolved Hide resolved
@kmpaul
Copy link
Contributor

kmpaul commented Feb 15, 2022

Just popping in here to say I think this is looking really cool!

Copy link
Collaborator

@jthielen jthielen left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a suggestion to not lose CF compliance just to make Pint happy.

xwrf/postprocess.py Show resolved Hide resolved
xwrf/config.yaml Outdated Show resolved Hide resolved
tests/test_postprocess.py Outdated Show resolved Hide resolved
@lpilz
Copy link
Collaborator Author

lpilz commented Feb 16, 2022

Thanks for the review :)

@lpilz
Copy link
Collaborator Author

lpilz commented Feb 16, 2022

Just popping in here to say I think this is looking really cool!

Thanks 😊

@lpilz
Copy link
Collaborator Author

lpilz commented Feb 16, 2022

Let's merge this PR quickly before it really gets out-of scope ;) Sorry for 9e37255 - fa8fbee, but this just bugged me...

Copy link
Member

@andersy005 andersy005 left a comment

Choose a reason for hiding this comment

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

Thank you, @lpilz! This looks great!

Copy link
Contributor

@kmpaul kmpaul left a comment

Choose a reason for hiding this comment

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

This looks great, @lpilz! Thanks for putting this together!

If ready, I say go ahead and merge.

@andersy005 andersy005 merged commit 23237fc into xarray-contrib:main Feb 16, 2022
@andersy005 andersy005 added the enhancement New feature or request label Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants