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

More comprehensive unit harmonization #116

Merged
merged 9 commits into from
Jul 28, 2023

Conversation

lpilz
Copy link
Collaborator

@lpilz lpilz commented Oct 24, 2022

Change Summary

Unit harmonization is improved by:

  • using a better map parsed from WRF Registries (yes, all of them, but not WPS)
    • translations are generated manually using custom external tool
    • includes all versions from WRFv4.0 onwards
    • makes bracket cleaning superfluous
  • extracting this map from the config yaml to avoid clutter

Related issue number

Checklist

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

@lpilz lpilz added the enhancement New feature or request label Oct 24, 2022
@lpilz lpilz modified the milestones: v0.0.2, v0.1 Oct 24, 2022
@lpilz lpilz requested a review from jthielen October 24, 2022 21:11
@lpilz
Copy link
Collaborator Author

lpilz commented Oct 24, 2022

There are some units which I didn't quite know how to translate:

  • savedtke12d - top level eddy conductivity from previous timestep (W/m.K)
    • is this W m-1 K-1 or W K m-1?
  • maxMF - Maximum mass-flux (neg: all dry, pos: moist) (m/s * area)
    • is this m3 s-1 or m-1 s-1 or even kg m2 s-1 or kg s-1 m-2?

@lpilz lpilz changed the title Amended unit harmonization More comprehensive unit harmonization Oct 27, 2022
@lpilz
Copy link
Collaborator Author

lpilz commented Nov 8, 2022

Small ping on this PR :)

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.

Sorry for the extremely long delay in getting around to reviewing this (last month has been kind of crazy for me)! The updated approach taken here looks good to me. Main suggested change would be better testing these changes:

  • add replacement tests for the bracket-related units for the tests removed from tests/test_postprocess.py
  • add new tests for each of the new categories of units handled in the expanded unit_harmonization_map

@jthielen
Copy link
Collaborator

As far as the units of uncertain translation go:

@lpilz
Copy link
Collaborator Author

lpilz commented Dec 28, 2022

Hey, happy holidays! Sorry for the long delay on my part now... Didn't have too much time to spend on this until now due to AGU.

I added the changes, however I don't quite know what you would like to see here.

add new tests for each of the new categories of units handled in the expanded unit_harmonization_map

What are the new categories you're referring to?

@lpilz
Copy link
Collaborator Author

lpilz commented Feb 3, 2023

Small ping here :)

@jthielen
Copy link
Collaborator

jthielen commented Feb 3, 2023

Thanks for the ping on this, and sorry for the very long delay! Unfortunately I'm a bit swamped with prelim-related stuff at the moment, so I won't be able to get back to this until next week. But, hopefully will be able to then.

@lpilz
Copy link
Collaborator Author

lpilz commented Mar 7, 2023

Hey :) just a small reping. If and whenever you find the time of course!

@lpilz
Copy link
Collaborator Author

lpilz commented Jul 27, 2023

Hey @jthielen, I'm just going to go ahead and merge this if you don't mind.

@jthielen
Copy link
Collaborator

Hey @jthielen, I'm just going to go ahead and merge this if you don't mind.

Yes, please go ahead! Sorry for my continued inattentiveness. Hopefully things will be getting better soon.

@lpilz lpilz merged commit 48bc061 into xarray-contrib:main Jul 28, 2023
@lpilz lpilz deleted the better_unit_treatment branch July 28, 2023 18:50
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.

2 participants