CAM computes enthalpy to send to BLOM by mvertens · Pull Request #62 · NorESMhub/OSLO_AERO

mvertens · 2025-10-09T07:35:33Z

This PR supersedes PR #58

Resolve pcols / ncol conflict. Fix mismatch where some arrays in an equation used ':' as the first dimension while others used ':ncol'. This fails when ncol /= pcols which happens when there is an unfilled chunk on any task. Addresses NorESMhub/CAM#233 May fix NorESMhub/CAM#180

gold2718

Some questions, some optimization ideas, and at least one code section that does not look right.

src_cam/physpkg.F90

gold2718 · 2025-10-14T13:29:14Z

src_cam/physpkg.F90

+            ifld = pbuf_get_index('ENTHALPY_PREC_AC', errcode=i)
+            if (ifld>0) call pbuf_set_field(pbuf, ifld, 0._r8)


Why ask for ifld when there is already a module variable for it? Also, if ifld <= 0, isn't that an error since the pbuf field is added using the same logical (computer_enthalpy_flux)?

Suggested change

ifld = pbuf_get_index('ENTHALPY_PREC_AC', errcode=i)

if (ifld>0) call pbuf_set_field(pbuf, ifld, 0._r8)

if (enthalpy_prec_ac_idx > 0) then

call pbuf_set_field(pbuf, ifld, 0._r8)

else

call endrun('pbuf field, ENTHALPY_PREC_AC, not defined')

end if

Good point. Done.

I don't think we need the check for > 0 since the pbuf_add_field for ENTHALPY_PREC_AC is in this module and only occurs if compute_enthalpy_flux is .true.

gold2718 · 2025-10-14T16:32:17Z

src_cam/physpkg.F90

+  integer ::  enthalpy_prec_bc_idx = 0
+  integer ::  enthalpy_prec_ac_idx = 0
+  integer ::  enthalpy_evop_idx    = 0


These variables should be public, protected and used from check_energy.F90 and camsrfexch.F90 instead of calling pbuf_get_index there (or later in this module).

I have put these in phys_init.

gold2718 · 2025-10-14T16:34:04Z

src_cam/physpkg.F90

+  integer ::  enthalpy_evop_idx    = 0
+  integer ::  qcsedten_idx=0, qrsedten_idx=0
+  integer ::  qisedten_idx=0, qssedten_idx=0, qgsedten_idx=0
+  integer ::  qrain_mg_idx=0, qsnow_mg_idx=0


These variables should be public, protected and used from check_energy.F90 instead of calling pbuf_get_index there (or later in this module).

I have put setting this in phys_init.

gold2718 · 2025-10-17T12:31:28Z

src_cam/physpkg.F90

+                call pbuf_get_field(pbuf, qssedten_idx, qssedten)
+                qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:)-qssedten(:ncol,:)
+             endif
+             if (qgsedten_idx.gt.0) then


Please update syntax:

Suggested change

if (qgsedten_idx.gt.0) then

if (qgsedten_idx > 0) then

gold2718 · 2025-10-17T12:34:44Z

src_cam/physpkg.F90

+          qrain_mg_idx = pbuf_get_index('qrain_mg'     , errcode=i)
+          qsnow_mg_idx = pbuf_get_index('qsnow_mg'     , errcode=i)


These should also be moved to phys_init. Also, I think the errcode=i optional input should be dropped since the return code is not being checked. Without that, a failure results in a call to endrun while this version would fail silently.

gold2718 · 2025-10-17T12:35:52Z

src_cam/physpkg.F90

+       endif
+    endif


Please restore spaces (nit pick):

Suggested change

endif

endif

end if

end if

gold2718 · 2025-10-17T12:39:28Z

src_cam/physpkg.F90

+       call cnst_get_ind('Q', ixq)
+       call cnst_get_ind('CLDLIQ', ixcldliq)
+       call cnst_get_ind('CLDICE', ixcldice)


Bit of potential cleanup:
Move ixq, ixcldliq and ixcldice to be module variables, move the cnst_get_ind calls to phys_init (including from tphysbc), and remove them from the run-time routines.

gold2718 · 2025-10-17T12:41:38Z

src_cam/physpkg.F90

+      if (compute_enthalpy_flux) then
+         ! In first time-step tphysac variables need to be zero'd out
+         if (compute_enthalpy_flux) then


Something seems wrong here since the inner if statement is the same as the outer one. The comment mentions "first time-step" but the logic does not reflect that.

gold2718

Overall, this is much better!
There are still some items from the first review plus a few new ones.

gold2718 · 2025-11-04T14:12:46Z

src_cam/physpkg.F90

+       qcsedten_idx = pbuf_get_index('QCSEDTEN', ierr)
+       qrsedten_idx = pbuf_get_index('QRSEDTEN', ierr)
+       qisedten_idx = pbuf_get_index('QISEDTEN', ierr)
+       qssedten_idx = pbuf_get_index('QSSEDTEN', ierr)
+       qgsedten_idx = pbuf_get_index('QGSEDTEN', ierr)


You should either check the ierr return values or leave them out (which calls endrun for any field not found).

Suggested change

qcsedten_idx = pbuf_get_index('QCSEDTEN', ierr)

qrsedten_idx = pbuf_get_index('QRSEDTEN', ierr)

qisedten_idx = pbuf_get_index('QISEDTEN', ierr)

qssedten_idx = pbuf_get_index('QSSEDTEN', ierr)

qgsedten_idx = pbuf_get_index('QGSEDTEN', ierr)

qcsedten_idx = pbuf_get_index('QCSEDTEN')

qrsedten_idx = pbuf_get_index('QRSEDTEN')

qisedten_idx = pbuf_get_index('QISEDTEN')

qssedten_idx = pbuf_get_index('QSSEDTEN')

qgsedten_idx = pbuf_get_index('QGSEDTEN')

gold2718 · 2025-11-04T14:14:33Z

src_cam/physpkg.F90

+    real(r8), dimension(pcols,pver)   :: qrain_mg_macmic   , qsnow_mg_macmic
+    integer :: m_cnst
+    real(r8):: hflx_iref(pcols)
+    character(50) :: physparname


This should use len= and maybe a standard value:

Suggested change

character(50) :: physparname

character(len=CS) :: physparname

or

Suggested change

character(50) :: physparname

character(len=16) :: physparname

gold2718 · 2025-11-04T15:02:10Z

src_cam/physpkg.F90

+             if (qcsedten_idx > 0) then
+                call pbuf_get_field(pbuf, qcsedten_idx, qcsedten)
+                qrain_mg_macmic(:ncol,:) = qrain_mg_macmic(:ncol,:) - qcsedten(:ncol,:)
+             endif
+             if (qrsedten_idx > 0) then
+                call pbuf_get_field(pbuf, qrsedten_idx, qrsedten)
+                qrain_mg_macmic(:ncol,:) = qrain_mg_macmic(:ncol,:) - qrsedten(:ncol,:)
+             endif
+             if (qisedten_idx > 0) then
+                call pbuf_get_field(pbuf, qisedten_idx, qisedten)
+                qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qisedten(:ncol,:)
+             endif
+             if (qssedten_idx > 0) then
+                call pbuf_get_field(pbuf, qssedten_idx, qssedten)
+                qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qssedten(:ncol,:)
+             endif
+             if (qgsedten_idx > 0) then
+                call pbuf_get_field(pbuf, qgsedten_idx, qgsedten)
+                qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qgsedten(:ncol,:)
+             endif


If you apply the suggestion back at 968 - 972, you can save a lot of logic here.

Suggested change

if (qcsedten_idx > 0) then

call pbuf_get_field(pbuf, qcsedten_idx, qcsedten)

qrain_mg_macmic(:ncol,:) = qrain_mg_macmic(:ncol,:) - qcsedten(:ncol,:)

endif

if (qrsedten_idx > 0) then

call pbuf_get_field(pbuf, qrsedten_idx, qrsedten)

qrain_mg_macmic(:ncol,:) = qrain_mg_macmic(:ncol,:) - qrsedten(:ncol,:)

endif

if (qisedten_idx > 0) then

call pbuf_get_field(pbuf, qisedten_idx, qisedten)

qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qisedten(:ncol,:)

endif

if (qssedten_idx > 0) then

call pbuf_get_field(pbuf, qssedten_idx, qssedten)

qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qssedten(:ncol,:)

endif

if (qgsedten_idx > 0) then

call pbuf_get_field(pbuf, qgsedten_idx, qgsedten)

qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qgsedten(:ncol,:)

endif

call pbuf_get_field(pbuf, qcsedten_idx, qcsedten)

qrain_mg_macmic(:ncol,:) = qrain_mg_macmic(:ncol,:) - qcsedten(:ncol,:)

call pbuf_get_field(pbuf, qrsedten_idx, qrsedten)

qrain_mg_macmic(:ncol,:) = qrain_mg_macmic(:ncol,:) - qrsedten(:ncol,:)

call pbuf_get_field(pbuf, qisedten_idx, qisedten)

qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qisedten(:ncol,:)

call pbuf_get_field(pbuf, qssedten_idx, qssedten)

qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qssedten(:ncol,:)

call pbuf_get_field(pbuf, qgsedten_idx, qgsedten)

qsnow_mg_macmic(:ncol,:) = qsnow_mg_macmic(:ncol,:) - qgsedten(:ncol,:)

gold2718 · 2025-11-12T10:14:48Z

src_cam/physpkg.F90

+      call get_hydrostatic_energy(state%q(1:ncol,1:pver,1:pcnst),.true.,          &
+           state%pdel(1:ncol,1:pver), cp_or_cv_dycore(:ncol,:,lchnk),             &
+           state%u(1:ncol,1:pver), state%v(1:ncol,1:pver), state%T(1:ncol,1:pver),&
+           vc_dycore, ptop=state%pintdry(1:ncol,1), phis = state%phis(1:ncol),    &
+           te = te_init(:ncol,1,lchnk), se=te_init(:ncol,2,lchnk), po=te_init(:ncol,3,lchnk), ke=te_init(:ncol,4,lchnk))


I do not think you need to pass the se, p0, or ke arguments as they are never used.
Only te_init(:,1,:) is used and I want to remove that second dimension (also see xxx comments in air_composition.F90). I realize this will not compile without also changing the CAM code but it seems reasonable.

Also, the lower bounds on some of these array references is not needed, can the use be made consistent?

Suggested change

call get_hydrostatic_energy(state%q(1:ncol,1:pver,1:pcnst),.true., &

state%pdel(1:ncol,1:pver), cp_or_cv_dycore(:ncol,:,lchnk), &

state%u(1:ncol,1:pver), state%v(1:ncol,1:pver), state%T(1:ncol,1:pver),&

vc_dycore, ptop=state%pintdry(1:ncol,1), phis = state%phis(1:ncol), &

te = te_init(:ncol,1,lchnk), se=te_init(:ncol,2,lchnk), po=te_init(:ncol,3,lchnk), ke=te_init(:ncol,4,lchnk))

call get_hydrostatic_energy(state%q(:ncol,:pver,:pcnst),.true., &

state%pdel(:ncol,1:pver), cp_or_cv_dycore(:ncol,:,lchnk), &

state%u(:ncol,:pver), state%v(:ncol,:pver), state%T(:ncol,:pver),&

vc_dycore, ptop=state%pintdry(:ncol,1), phis = state%phis(:ncol), &

te = te_init(:ncol,lchnk))

gold2718 · 2025-11-12T12:48:25Z

src_cam/physpkg.F90


+      if (compute_enthalpy_flux) then
+         ! In first time-step tphysac variables need to be zero'd out
+         call pbuf_set_field(pbuf, ifld, 0._r8)


What is ifld supposed to be here? ifld is set to the index of different pbuf fields above, which is this? Should it be an enthalpy-specific pbuf? All of them? Right now, it is probably FRACIS but who knows for sure?

gold2718 · 2025-11-12T14:27:46Z

src_cam/physpkg.F90

+    if (compute_enthalpy_flux) then
+       call cam_export(state, cam_out, pbuf, cam_in=cam_in)
+    else
+       call cam_export(state, cam_out, pbuf)
+    end if


I am not a fan of this method of communicating the enthalpy flag to cam_export. Instead of using the passing of cam_in, can you make cam_in a required (not optional) input to cam_export and use compute_enthalpy_flux in place of present(cam_in)?

Suggested change

if (compute_enthalpy_flux) then

call cam_export(state, cam_out, pbuf, cam_in=cam_in)

else

call cam_export(state, cam_out, pbuf)

end if

call cam_export(state, cam_out, pbuf, cam_in)

Øyvind Seland and others added 7 commits June 14, 2025 19:14

Modification needed for improved enthalpy calculations

a1ccab7

Merge remote-tracking branch 'origin/noresm_develop' into eneplusZM_mods

089b813

Merge remote-tracking branch 'origin/noresm_develop' into eneplusZM_mods

cce4ecc

removed references to tht and pel in physpkg.F90

cba0c76

update for changes to qneg4 that make seflx an optional argument

5a03092

added new if statements for compute_enthalpy_flux

442f15b

mvertens requested a review from gold2718 October 9, 2025 07:35

mvertens marked this pull request as draft October 9, 2025 07:35

gold2718 requested changes Oct 17, 2025

View reviewed changes

mvertens marked this pull request as ready for review October 20, 2025 12:20

mvertens mentioned this pull request Oct 20, 2025

Modification needed for improved enthalpy calculations #58

Closed

address issues raised in PR review

55cd786

mvertens requested a review from gold2718 October 20, 2025 19:52

mvertens assigned gold2718 Oct 20, 2025

gold2718 requested changes Nov 12, 2025

View reviewed changes

		ifld = pbuf_get_index('ENTHALPY_PREC_AC', errcode=i)
		if (ifld>0) call pbuf_set_field(pbuf, ifld, 0._r8)

		qrain_mg_idx = pbuf_get_index('qrain_mg' , errcode=i)
		qsnow_mg_idx = pbuf_get_index('qsnow_mg' , errcode=i)

-       endif
-    endif
+       end if
+    end if

	character(50) :: physparname
	character(len=CS) :: physparname

	character(50) :: physparname
	character(len=16) :: physparname

Conversation

mvertens commented Oct 9, 2025

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants