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

Total energy/water fields missing second dimension in snapshots #1141

Closed
jimmielin opened this issue Sep 5, 2024 · 10 comments
Closed

Total energy/water fields missing second dimension in snapshots #1141

jimmielin opened this issue Sep 5, 2024 · 10 comments
Assignees
Labels
bug Something isn't working correctly

Comments

@jimmielin
Copy link
Member

What happened?

The following fields from physics_state have two dimensions, but the second dimension is currently not saved in CAM snapshots:

te_ini,         &! vertically integrated total (kinetic + static) energy of initial state
te_cur,         &! vertically integrated total (kinetic + static) energy of current state
tw_ini,         &! vertically integrated total water of initial state
tw_cur           ! vertically integrated total water of new state

The second dimension is phys_te_idx (or 1) to represent CAM physics total energy, and dyn_te_idx (or 2) to represent dycore total energy computed in physics.

Proposed fix

As discussed with @cacraigucar @nusbaume @peverwhee:

Taking te_ini as an example (this would be repeated for the 4 variables with issues), I am proposing to:

  1. write these fields into separate variables in the CAM snapshot file:
  • state_te_ini_phys for CAM physics total energy
  • state_te_ini_dyn for dycore total energy computed in physics
    By modifying only the snapshot infra, i.e. cam_snapshot_common.F90 in CAM (and corresponding read in CAM-SIMA). No other changes to CAM code will be made.
  1. Use standard names vertically_integrated_energies_of_initial_state_in_cam and vertically_integrated_energies_of_initial_state_in_dynamics for te_ini_phys and te_ini_dyn respectively.

Currently, the CCPP standard name spreadsheet states te_ini's standard name is vertically_integrated_energies_of_initial_state (no suffix). CAM-SIMA code reads state_te_ini into vertically_integrated_energies_of_initial_state_in_cam.

@PeterHjortLauritzen can I ask if these standard names, particularly suffixes (_in_cam and _in_dynamics) look good to you?

Thanks!

What are the steps to reproduce the bug?

Save CAM snapshot and observe state_te_ini, state_te_cur, state_tw_ini, state_tw_cur fields, which are saved only with (time, ncol) dimensions.

What CAM tag were you using?

cam6_4_028

What machine were you running CAM on?

CISL machine (e.g. cheyenne)

What compiler were you using?

Intel

Path to a case directory, if applicable

No response

Will you be addressing this bug yourself?

Yes

Extra info

No response

@jimmielin jimmielin added the bug Something isn't working correctly label Sep 5, 2024
@cacraigucar
Copy link
Collaborator

@jimmielin - I think the suffix should be "due_to", which is what we've used elsewhere in the standard names (you are new to these standard names, so no way you would know this). So I would propose due_to_physics and due_to_dynamics be added to the end of each of the current standard names.

@PeterHjortLauritzen
Copy link
Collaborator

In this particular case the "due_to" might be misleading because state_te_ini_phys and state_te_ini_dyn is the total energy in each physics column using the CAM physics total energy formula and the dynamical core (SE, MPAS, FV, whichever supported dycore is used) total energy formula, respectively.

Will this standard name be too long?

vertically_integrated_total_energy_of_initial_state_using_physics_energy_formula
vertically_integrated_total_energy_of_initial_state_using_dycore_energy_formula

@jimmielin
Copy link
Member Author

Thanks @cacraigucar, @PeterHjortLauritzen for your feedback. I believe vertically_integrated_total_energy_of_initial_state_using_physics_energy_formula should fit.

Do we want to use _using_physics_energy_formula for the tw_ini tw_cur (total water) fields as well? So these would be

  • vertically_integrated_total_water_of_initial_state_using_physics_energy_formula
  • vertically_integrated_total_water_of_initial_state_using_dycore_energy_formula
  • vertically_integrated_total_water_of_current_state_using_physics_energy_formula
  • vertically_integrated_total_water_of_current_state_using_dycore_energy_formula

The four fields come from the same subroutine (get_hydrostatic_energy_1hd) in cam_thermo.F90, I think the same suffixes could work.

@cacraigucar
Copy link
Collaborator

@PeterHjortLauritzen There are plenty of standard names which are longer - so no worries on length. Since that accurately describes the variables, then I suggest we go with you proposed.

@PeterHjortLauritzen
Copy link
Collaborator

PeterHjortLauritzen commented Sep 5, 2024

You just uncovered "bad" code (probably introduced by me!)

Total water vapor should not depend on the physics or dycore energy formula, as it is a vertical integral of mass that is unambiguous. However, I see in the code that it has dimensions of tw_ini(:pcols, 2). I don't think tw_ini(:, 2) is used anywhere in the code, though. So some code changes are needed in order to not introduce two standard names when only one is needed.

So ... first of all, the longname should probably be

vertically_integrated_moist_air_and_condensed_water_of_current_state

We had long discussions on how to specify moist mixing ratios and ended up with using "mixing_ratio_wrt_moist_air_and_condensed_water" so I think we should be consistent with that in the long name.

@nusbaume do you agree?

Second, code changes are need in check_energy to remove the use of state%tw_ini(1:ncol,dyn_te_idx). Basically removing

H2O = state%tw_ini(1:ncol,dyn_te_idx)

from the argument list when calling get_hydrostatic_energy and

 state%tw_ini(1:ncol,dyn_te_idx) = state%tw_ini(1:ncol,phys_te_idx)

Let me know if you need help with the code changes ...

@jimmielin
Copy link
Member Author

Thanks Peter, that makes sense. So I think I can:

  • remove the second dimension of state%tw_ini and state%tw_cur completely
  • change calls to remove second dimension e.g., H2O = state%tw_ini(1:ncol)
  • keep the snapshot output of tw_ini and tw_cur as-is, and change the CAM-SIMA standard name to vertically_integrated_moist_air_and_condensed_water_of_current_state

this way we can have two less snapshot variables since state%tw_ini and state%tw_cur don't have different formulas.

This goes a little beyond just changing the snapshot code, so I can make the changes and verify (@nusbaume I think I might have to run regression tests after all!) these changes are bit-for-bit. (Looking at the code path I think it should)

@PeterHjortLauritzen
Copy link
Collaborator

Exactly!

Apologies for creating extra work but this will avoid introducing standard names that are redundant ...

jimmielin added a commit to jimmielin/CAM-SIMA that referenced this issue Sep 6, 2024
- read ncdata variables te_ini_phys, te_ini_dyn, te_cur_phys, te_cur_dyn
- update to standard names per discussion in ESCOMP/CAM#1141
@jimmielin
Copy link
Member Author

Thanks Peter.

PRs updated in CAM for new snapshot variables and remove tw_ini, tw_cur's second dimensions: #1142

and CAM-SIMA to read the new snapshot variables and update standard names: ESCOMP/CAM-SIMA#296

jimmielin added a commit to jimmielin/CAM-SIMA that referenced this issue Sep 16, 2024
- read ncdata variables te_ini_phys, te_ini_dyn, te_cur_phys, te_cur_dyn
- update to standard names per discussion in ESCOMP/CAM#1141
@nusbaume
Copy link
Collaborator

Hi @jimmielin! After chatting with @PeterHjortLauritzen I believe we'll want to make one change to the tw_X standard names. Basically they should be renamed to the following:

vertically_integrated_water_vapor_and_condensed_water_of_current_state
vertically_integrated_water_vapor_and_condensed_water_of_initial_state

Or in other words we need to replace moist_air with water_vapor in the standard names. This way we can make it clear that the mass of dry air is NOT being included in these two quantities. Thanks!

@jimmielin
Copy link
Member Author

Thanks @nusbaume, I've updated the PR at ESCOMP/CAM-SIMA#296. I think the CAM-SIMA repo is the only place where these standard names appear right now, but please let me know if anywhere else needs to be changed.

@github-project-automation github-project-automation bot moved this from To Do to Done in CAM Development Sep 19, 2024
jimmielin added a commit to ESCOMP/CAM-SIMA that referenced this issue Sep 26, 2024
…date standard names (#296)

Companion issue in CAM: ESCOMP/CAM#1141

Companion PR in CAM: ESCOMP/CAM#1142
this PR in CAM will update snapshots with new variables.

Updates to CAM-SIMA:
- read ncdata variables te_ini_phys, te_ini_dyn, te_cur_phys, te_cur_dyn
- update to standard names per discussion in
ESCOMP/CAM#1141
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly
Projects
Status: Done
Development

No branches or pull requests

4 participants