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

Updates to CESM-FV Reader #228

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

noribeth-m
Copy link

I made some updates to the CESM-FV reader to have the capability to calculate the layer thickness (dz_m) if the interface pressures (hyai, hybi) are provided in the model output. The script will now calculate the variable, 'pres_pa_int', and use this variable to calculate the interface layer height, which is then used to calculate the layer thickness. These calculations are based on the default vertical structure of CESM, where the pressure is in increasing order and altitude in decreasing order. Once this is calculated, the script reorders the vertical levels so that the surface is first. This is done by default in the original code, but I added a line so that 'pres_pa_int' is also treated this way, as the original code does this based on vertical dimension 'z', which is not what is used for 'pres_pa_int' since it has an extra vertical level (33 instead of 32). 'pres_pa_int' has a vertical dimension called 'ilev'. This is currently only used in the CESM_FV reader, but is saved in the processed dataset. Moving forward, if needed, renaming the 'ilev' dimension to something like 'z_i' may be more consistent with the monetio/melodies-monet formatting, but not necessary for now.

In addition to this, the pyresample dependency was removed and now deals with the wrap longitudes using numpy.

Copy link
Member

@zmoon zmoon left a comment

Choose a reason for hiding this comment

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

Hi @noribeth-m, some initial comments here. Also maybe we can come up with a new test file/case by selecting a few levels from data here since currently we're only testing a surf-only case.

@zmoon
Copy link
Member

zmoon commented Feb 25, 2025

@noribeth-m the automatic formatters want some changes. If you want to address this yourself, install pre-commit in your Python env, then in the repo run

pre-commit install
pre-commit run --all-files

and commit the result.

Or I can do it for you if you'd prefer.

@noribeth-m
Copy link
Author

Hi @zmoon! I installed and ran the pre-commit. It says that the _cesm_fv_mm.py file was reformatted, but I still get "Failed" in 2 places (see image attached). I'm not sure what I'm supposed to do from here.

Screenshot 2025-02-25 at 1 17 31 PM

@zmoon
Copy link
Member

zmoon commented Feb 25, 2025

@noribeth-m if you run pre-commit run --all-files again do you get all green? If so you can commit the result with git and then the format check here in GitHub will be happy.

@noribeth-m
Copy link
Author

@zmoon Thanks! That worked and the checks have passed.

Copy link
Member

@zmoon zmoon left a comment

Choose a reason for hiding this comment

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

A few more comments

Comment on lines +351 to +354
raise Exception(
"Expected default CESM behaviour:"
+ "pressure levels should be in increasing order, height in decreasing order"
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise Exception(
"Expected default CESM behaviour:"
+ "pressure levels should be in increasing order, height in decreasing order"
)
raise ValueError(
"Expected default CESM behaviour "
"(pressure levels should be in increasing order, height in decreasing order)"
)

Comment on lines +71 to +75
var_list = var_list + ["pres_pa_mid"]
if "Z3" not in dset_load.keys():
warnings.warn("Geopotential height Z3 is not in model keys. Assuming hydrostatic runs")
dset_load["Z3"] = _calc_hydrostatic_height(dset_load)
var_list = var_list + ["alt_msl_m_mid"]
Copy link
Member

Choose a reason for hiding this comment

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

It looks like pres_pa_mid and alt_msl_m_mid are also added to the var list later. So perhaps remove the two lines here. xarray seems to just ignore duplicates but it's confusing to have it added twice.

var_list = var_list + ["alt_msl_m_mid"]

# calc layer thickness if hyai and hybi exist
if "hyai" and "hybi" in dset_load.keys():
Copy link
Member

Choose a reason for hiding this comment

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

As it is, it's only actually checking for 'hybi' (since 'hyai' is always true). This is the concise way to check for both:

Suggested change
if "hyai" and "hybi" in dset_load.keys():
if {"hyai", "hybi"} <= dset_load.keys():

if "hyai" and "hybi" in dset_load.keys():
dset_load["pres_pa_int"] = _calc_pressure_i(dset_load)
dset_load["dz_m"] = _calc_layer_thickness_i(dset_load)
var_list.append("dz_m")
Copy link
Member

Choose a reason for hiding this comment

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

You could add an else branch here with a warning message about skipping this calculation.

Comment on lines +383 to +384
because it has a dimension of 'ilev' instead of 'z'. Add a correction
for this last part.
Copy link
Member

Choose a reason for hiding this comment

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

👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants