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

CCPPize dme_adjust #211

Open
wants to merge 8 commits into
base: development
Choose a base branch
from

Conversation

cacraigucar
Copy link
Collaborator

@cacraigucar cacraigucar commented Feb 26, 2025

Originator(s): cacraig

Description (include issue title and the keyword ['closes', 'fixes', 'resolves'] and issue number):
closes #103 - CCPP-ized version of physics_dme_adjust

List all namelist files that were added or changed: N/A

List all files eliminated and why: (created conservation_adjust subdirectory and moved check_energy subdirectory into it)

R100 schemes/check_energy/check_energy_chng.F90 schemes/conservation_adjust/check_energy/check_energy_chng.F90
R100 schemes/check_energy/check_energy_chng.meta schemes/conservation_adjust/check_energy/check_energy_chng.meta
R100 schemes/check_energy/check_energy_chng_namelist.xml schemes/conservation_adjust/check_energy/check_energy_chng_namelist.xml
R100 schemes/check_energy/check_energy_fix.F90 schemes/conservation_adjust/check_energy/check_energy_fix.F90
R100 schemes/check_energy/check_energy_fix.meta schemes/conservation_adjust/check_energy/check_energy_fix.meta
R100 schemes/check_energy/check_energy_gmean/check_energy_gmean.F90 schemes/conservation_adjust/check_energy/check_energy_gmean/check_energy_gmean.F90
R100 schemes/check_energy/check_energy_gmean/check_energy_gmean.meta schemes/conservation_adjust/check_energy/check_energy_gmean/check_energy_gmean.meta
R100 schemes/check_energy/check_energy_save_teout.F90 schemes/conservation_adjust/check_energy/check_energy_save_teout.F90
R100 schemes/check_energy/check_energy_save_teout.meta schemes/conservation_adjust/check_energy/check_energy_save_teout.meta
R100 schemes/check_energy/check_energy_scaling.F90 schemes/conservation_adjust/check_energy/check_energy_scaling.F90
R100 schemes/check_energy/check_energy_scaling.meta schemes/conservation_adjust/check_energy/check_energy_scaling.meta
R100 schemes/check_energy/check_energy_zero_fluxes.F90 schemes/conservation_adjust/check_energy/check_energy_zero_fluxes.F90
R100 schemes/check_energy/check_energy_zero_fluxes.meta schemes/conservation_adjust/check_energy/check_energy_zero_fluxes.meta
R100 schemes/check_energy/dycore_energy_consistency_adjust.F90 schemes/conservation_adjust/check_energy/dycore_energy_consistency_adjust.F90
R100 schemes/check_energy/dycore_energy_consistency_adjust.meta schemes/conservation_adjust/check_energy/dycore_energy_consistency_adjust.meta

List all files added and what they do:
A test/test_suites/suite_dme_adjust.xml
A schemes/conservation_adjust/dme_adjust/dme_adjust.F90
A schemes/conservation_adjust/dme_adjust/dme_adjust.meta

List all existing files that have been modified, and describe the changes:
(Helpful git command: git diff --name-status development...<your_branch_name>)
M suites/suite_cam7.xml
M suites/suite_kessler.xml
M suites/suite_tj2016.xml

List all automated tests that failed, as well as an explanation for why they weren't fixed:

Is this an answer-changing PR? If so, is it a new physics package, algorithm change, tuning change, etc?

If yes to the above question, describe how this code was validated with the new/modified features:

@cacraigucar cacraigucar added the enhancement New feature or request label Feb 26, 2025
@cacraigucar cacraigucar self-assigned this Feb 26, 2025
@cacraigucar cacraigucar changed the base branch from main to development February 26, 2025 21:58
Copy link
Member

@jimmielin jimmielin left a comment

Choose a reason for hiding this comment

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

Thanks @cacraigucar! I had two clarification questions but maybe they are outside the scope of the PR, I am not sure, depending what the plans for dme_adjust in SIMA are.

Comment on lines +55 to +57
real(kind_phys), intent(in) :: qini(:,:) ! initial specific humidity
real(kind_phys), intent(in) :: liqini(:,:) ! initial total liquid
real(kind_phys), intent(in) :: iceini(:,:) ! initial total ice
Copy link
Member

Choose a reason for hiding this comment

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

I suppose these are currently read from the snapshot but when SIMA runs standalone there might be the need for an init routine (or change in CAM-SIMA if q/liq/ice needs to be recorded at somewhere outside of physics) to initialize those?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nusbaume - what are your thoughts on this? These variables are actually initialized in tphysbc (not the tphysac layer where the run routine is called). I'd thought about writing an initialization routine, but this would require having a separate module for it. When I mentioned there are going to be a number of little routines that may need to be added to the main SDF's when we have all the physics translated, this is a concrete example of what I meant. I figure translating the limited code which resides in tphysac and tphysbc will be one of the final steps.

Copy link
Member

Choose a reason for hiding this comment

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

I am happy to discuss this in more detail (including whether to do this conversion now or later) but I realized I bumped into a similar issue in rk_stratiform. The "trick" I used (to permit this field to be both readable from a snapshot and initializable in a full CAM-SIMA run) is to set <initial_value>-1.0_kind_phys</initial_value> and detect this negative value in the (timestep?) initialization routine of this scheme, or if you need it to be outside of physics since it's for "before coupling", it could be in SIMA itself. In fact, rk_stratiform.F90 in ESCOMP/CAM already does this (but uses HUGE(1.0_r8) for safety)

   ! check that qcwat and tcwat were initialized; if not then do it now.
   if (qcwat(1,1) == huge(1._r8)) then
      qcwat(:ncol,:) = state%q(:ncol,:,1)
   end if
   if (tcwat(1,1) == huge(1._r8)) then
      tcwat(:ncol,:) = state%t(:ncol,:)
   end if

Since ESCOMP/CAM-SIMA#365 was developed, the model is capable of reading the actual, physical value from the snapshot when available (and thus no negatives), and if not (in an actual run), it'll have a negative value you can capture at timestep_init (or elsewhere) and properly initialize.

@@ -34,7 +34,7 @@
<!-- Update cp/cv for energy computation based in updated water variables -->
<scheme>thermo_water_update</scheme>

<!-- <scheme>physics_dme_adjust</scheme> -->
<!-- <scheme>dme_adjust</scheme> -->
Copy link
Member

Choose a reason for hiding this comment

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

I presume that this is not enabled because CAM-SIMA cannot run dme_adjust standalone due to qini/liqini/iceini not being set, could you confirm? It would be good to make a note of the reason why this will remain disabled, after this scheme is available, to signal that this is intentional.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @cacraigucar thanks for adding the comment, I think you may have missed a ! in the xml comment syntax (should be <!-- starting instead of <--)

Copy link
Member

@jimmielin jimmielin left a comment

Choose a reason for hiding this comment

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

Thanks @cacraigucar! just a couple of minor changes while we wait for discussion on the qini/liqini/iceini

@@ -34,7 +34,7 @@
<!-- Update cp/cv for energy computation based in updated water variables -->
<scheme>thermo_water_update</scheme>

<!-- <scheme>physics_dme_adjust</scheme> -->
<!-- <scheme>dme_adjust</scheme> -->
Copy link
Member

Choose a reason for hiding this comment

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

Hi @cacraigucar thanks for adding the comment, I think you may have missed a ! in the xml comment syntax (should be <!-- starting instead of <--)


<-- COMMENTED OUT until qini/liqini/iceini have initialization routines -->
Copy link
Member

Choose a reason for hiding this comment

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

Same issue regarding the comment syntax here (should be <!-- instead of <--)

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.

CCPP-ized version of physics_dme_adjust
2 participants