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

Added earth-relative wind field calculation to base diagnostics #100

Merged
merged 4 commits into from
Sep 16, 2022

Conversation

lpilz
Copy link
Collaborator

@lpilz lpilz commented Sep 16, 2022

Change Summary

This might not be the best way to implement this, but I think we should also include earth-relative wind field calculation if we already have the other diagnostics. What are your opinions?

Checklist

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

PS:

For me, locally, this change causes the test tests/test_accessors.py:15 test_postprocess[lambert_conformal-lambert_conformal_conic] to fail, though I think this is an unrelated bug. However, I don't know enough about cf_xarray to debug this.

The error is:

>       assert 'z' in standard_names['atmosphere_hybrid_sigma_pressure_coordinate']                                                                                                            
E       AssertionError: assert 'z' in ['z_stag']                                                                                                                                               

xwrf/postprocess.py Outdated Show resolved Hide resolved
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.

Mostly looks good! I initially thought that grid-relative/earth-relative conversion would be the kind of general-purpose thing to leave to MetPy to handle, but likewise to geopotential to geopotential height, this looks to have enough WRF-specific nuance to justify handling here.

A couple small suggestions:

  • Update the documentation of postprocess's Parameters to account for these new fields and that unlike the other diagnostic variables, the grid-relative winds are not dropped when drop_diagnostic_variable_components is true.
  • Use expected input type for _destag_variable (see suggestion below)

xwrf/postprocess.py Outdated Show resolved Hide resolved
@jthielen jthielen added the enhancement New feature or request label Sep 16, 2022
lpilz and others added 3 commits September 16, 2022 23:22
Co-authored-by: Anderson Banihirwe <axbanihirwe@gmail.com>
Co-authored-by: jthielen <jon@thielen.science>
@lpilz lpilz requested a review from jthielen September 16, 2022 21:48
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! I'll hold off merging in case you wanted further reviews from anyone else or to iterate on this in PR form still. That being said, if you want to get this in for v0.0.2 and build tutorial/blog post components around it, feel free to merge.

@lpilz
Copy link
Collaborator Author

lpilz commented Sep 16, 2022

I think I'll merge pursuant to your and @andersy005's review. Thanks, guys!

@lpilz lpilz merged commit bbd2669 into xarray-contrib:main Sep 16, 2022
@jthielen jthielen added this to the v0.0.2 milestone Sep 16, 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.

3 participants