EAMxx: Output P0 scalar variable alongside lev/ilev geometry data#8405
Conversation
Ensures IO will take care of outputing it (if geo data is requested)
d1c296f to
60d88cc
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds the CF-required scalar reference pressure variable (P0) alongside hybrid sigma-pressure vertical coordinate geometry (lev/ilev) so downstream tooling can correctly interpret hybrid coordinates.
Changes:
- Added
P0toio_metadata.csvwithlong_name = "reference pressure". - Introduced
FieldLayout::scalar()to represent 0-D (scalar) fields. - Ensured
P0geometry data is created and populated fromphysics::Constants<Real>::P0.valuewhen building/adding vertical coordinate geo data.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| components/eamxx/src/share/util/io_metadata/io_metadata.csv | Registers P0 metadata so IO can attach a descriptive long_name. |
| components/eamxx/src/share/field/field_layout.hpp | Adds a scalar (0-D) layout helper used to define P0 as a scalar field. |
| components/eamxx/src/share/data_managers/mesh_free_grids_manager.cpp | Creates and populates P0 in mesh-free grids to keep geo-data behavior consistent. |
| components/eamxx/src/dynamics/homme/homme_grids_manager.cpp | Creates and populates P0 in Homme physics grid and uses it to compute lev/ilev. |
|
@chengzhuzhang I don't know if you are familiar with the eamxx code, but I want to make sure that adding @AaronDonahue the PR adds P0 to the geo data of the grid, since we already have an automatic mechanism that writes geo data to file (including long name, units, etc). |
czender
left a comment
There was a problem hiding this comment.
@bartgol I added myself as a reviewer because I do not see where in this PR the CF formula_terms attribute has been added to lev and ilev coordinates. Same for standard_name. Perhaps those are in a different PR? If so, my concerns have been addressed. If not, then this PR is insufficient to address the original problem. Writing those attributes is also necessary to resolve the problem.
|
@bartgol thanks for getting to it so quickly. Sorry that I didn't make it clear that both formula_terms and standard_name attribute needs to be added, following the example from EAM. I just updated the issue description, and we may want to summon copilot for the additional work. @czender thank you for catch this. |
|
Ah, thanks. I did not get that we needed the formula attribute as well. I can easily add it. |
|
Side q: none of our vars have the attribute |
|
@czender @chengzhuzhang did I address all the concerns? Is there any other metadata you'd like me to add? |
|
@bartgol CF says "If the units attribute value is a valid pressure unit the default value of the positive attribute is down." |
Is the CF guidance to add this attribute only to the vert coord variable? Or to all variables with vertical levels? Edit: nvm, the AI bot answered that for me |
…v output variables
283777b to
6e06de4
Compare
|
Ok, I fixed the var name, and added the |
chengzhuzhang
left a comment
There was a problem hiding this comment.
The code change looks good to me! Thanks @bartgol .
Add metadata to vertical hybrid coordinates variables (required by NCO) [BFB]
CF conventions require a scalar reference pressure variable (
P0) to accompany hybrid sigma-pressure coordinate variables (lev/ilev). Its absence breaks downstream tools (e.g.,ncclimo, vertical interpolation workflows) that work fine with EAM output.Changes
io_metadata.csv: AddedP0entry withlong_name = "reference pressure".homme_grids_manager.cppandmesh_free_grids_manager.cpp: addP0as a 0-dim geo data field wheneverlev/ilevgeo data fields are added. Get value fromphysics::Constants<Real>. This ensures IO will automatically add it whenever thelev/ilevgeo data fields are added to the output stream.Luca: I edited this, as I changed the impl to a different strategy.