Dev011526 #14
Dev011526 #14
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the codebase to use branded_variable_name instead of physical_parameter.name for CMIP7 variable identification, preventing naming conflicts. The changes also include code cleanup, support for multiple pressure level grids (plev19/plev39), and removal of unused functionality.
Changes:
- Updated variable references from
physical_parameter.nametobranded_variable_name.namethroughout the codebase - Extended vertical interpolation to support multiple pressure grids (plev19, plev39) via a new
targetparameter - Renamed all variables in
cesm_to_cmip7.yamlto use branded variable names as keys - Removed unused imports and deprecated command-line options
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/monthly_cmor.py | Updated to use branded_variable_name.name for variable identification; removed unused imports and deprecated CLI flags |
| cmip7_prep/vertical.py | Renamed to_plev19 to to_plev with new target parameter for flexible pressure grid support |
| cmip7_prep/regrid.py | Added zonal_mean_on_pressure_grid function; reorganized code sections; updated INTENSIVE_VARS list |
| cmip7_prep/pipeline.py | Updated imports and logic to use new to_plev function with target parameter |
| cmip7_prep/mom6_static.py | Removed unused sftof extraction code |
| cmip7_prep/data/cesm_to_cmip7.yaml | Renamed all variable keys to use branded variable names; added new variable definitions |
| cmip7_prep/cmor_writer.py | Updated to use branded_variable_name for variable lookup and writing |
| - cesm_var: SOILICE | ||
|
|
||
| mrsos: | ||
| mrsol_tavg-d10cm-hxy-lnd: |
There was a problem hiding this comment.
This variable name mrsol_tavg-d10cm-hxy-lnd appears to be a duplicate key with mrsol_tavg-sl-hxy-lnd (line 359), which will cause the first definition to be overwritten. These should have distinct branded variable names.
| "long_name": cfg.get("long_name", None), | ||
| "standard_name": cfg.get("standard_name", None), | ||
| "levels": cfg.get("levels", None), | ||
| "branded_variable_name": cmip7name, |
There was a problem hiding this comment.
The variable cmip7name is not defined in the visible context. This will cause a NameError at runtime. Verify that cmip7name should be defined earlier in the function or use the appropriate variable reference such as cmip_var.branded_variable_name.name.
There was a problem hiding this comment.
cmip7name is defined at line 289
Co-authored-by: Copilot <[email protected]>
| # Example usage (in docstring): | ||
| # ds = xr.open_dataset("input.nc") | ||
| # zonal_p39 = zonal_mean_on_pressure_grid(ds, "ta", tables_path="cmip7-cmor-tables/tables") | ||
| # cmip7_prep/regrid.py |
There was a problem hiding this comment.
The comment at lines 1-3 appears to be example usage that should be in a docstring rather than as file header comments. This content should be moved into the docstring of the zonal_mean_on_pressure_grid function (lines 126-152) as an Examples section, or removed if not needed as a file header.
| # Example usage (in docstring): | |
| # ds = xr.open_dataset("input.nc") | |
| # zonal_p39 = zonal_mean_on_pressure_grid(ds, "ta", tables_path="cmip7-cmor-tables/tables") | |
| # cmip7_prep/regrid.py |
| return ds_out | ||
|
|
||
|
|
||
| # Variables treated as "intensive" → prefer bilinear when available. |
There was a problem hiding this comment.
This comment on line 111 is misplaced and appears to be a leftover fragment. It describes INTENSIVE_VARS (defined at lines 49-71) but is positioned just before the zonal_mean_on_pressure_grid function definition. Either remove this comment or ensure it's placed near the INTENSIVE_VARS definition where it belongs.
| positive: up | ||
| sources: | ||
| - cesm_var: FSUTOA | ||
| - cesm_var: FLUT |
There was a problem hiding this comment.
The source variable for rsut has been changed from FSUTOA to FLUT. This appears to be an error as rsut is shortwave radiation at TOA (top of atmosphere upwelling), which should come from a shortwave variable like FSUTOA, not a longwave variable like FLUT. This change likely introduces incorrect data mapping.
| - cesm_var: FLUT | |
| - cesm_var: FSUTOA |
| - cesm_var: SOILICE | ||
|
|
||
| mrsos: | ||
| mrsol_tavg-d10cm-hxy-lnd: |
There was a problem hiding this comment.
The variable key mrsol_tavg-d10cm-hxy-lnd appears to be a duplicate or incorrectly named entry. There's already a mrsol_tavg-sl-hxy-lnd entry at line 359. Based on the source variable SOILWATER_10CM, this should likely be named mrsos_tavg-d10cm-hxy-lnd (moisture in upper portion of soil column) rather than mrsol (total soil moisture content).
| mrsol_tavg-d10cm-hxy-lnd: | |
| mrsos_tavg-d10cm-hxy-lnd: |
1 participant
updates cesm_to_cmip7.yaml to use cmip7 branded_variable_names so that conflicts are avoided.