sima0_30_001: Moving-mountain GWD: tilt/precip source options and directional stress diagnostics - #448
Merged
jimmielin merged 12 commits intoOct 2, 2026
Conversation
diagnostics to moving-mountain gravity wave drag - New opt-in movmtn_source values 3-5: tilt-layer-mean source, tilt+precip 6-parameter fit, and tilt+precip PySR cx17 fit (existing values 1=vorticity and 2=PBL momentum flux unchanged; default remains 1, so no answer changes for existing configurations). - Add taucd_west/east/south/north as new intent(out) diagnostics: cardinal- direction Reynolds stresses derived from the wave momentum-flux spectrum via gw_common's calc_taucd, exposed for history output. - Reconciled against current atmos_phys0_29_000 via 3-way merge from a 2-week-old local development branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cacraigucar
self-requested a review
September 24, 2026 17:31
jimmielin
self-requested a review
September 24, 2026 17:40
jimmielin
reviewed
Sep 24, 2026
jimmielin
left a comment
jimmielin
left a comment
Collaborator
There was a problem hiding this comment.
Some initial questions and comments.
Overall this looks good but I wonder about the significant comment removal from the original GW moving mountain code in this PR. @JulioTBacmeister, can I ask if this was intentional cleanup (e.g., the comments are stale, or not needed)? The amount of removed comments appear nontrivial to me, so I wanted to confirm if it is intentional or Claude took the decision to clean things up.
Collaborator
Author
|
Hi Haipeng,
I didn't tell Claude to NOT clean things up. I did clean some comments
myself (never with British spelling). I will check the code over and get
back to you shortly.
…On Thu, Sep 24, 2026 at 11:47 AM Haipeng Lin ***@***.***> wrote:
***@***.**** commented on this pull request.
Some initial questions and comments.
Overall this looks good but I wonder about the significant comment removal
from the original GW moving mountain code in this PR. @JulioTBacmeister
<https://github.com/JulioTBacmeister>, can I ask if this was intentional
cleanup (e.g., the comments are stale, or not needed)? The amount of
removed comments appear nontrivial to me, so I wanted to confirm if it is
intentional or Claude took the decision to clean things up.
------------------------------
In doc/ChangeLog
<#448 (comment)>
:
> @@ -1,5 +1,61 @@
===============================================================
+Tag name: TBD (assigned at merge)
A note that doc/ChangeLog in atmos_phys is deprecated so there's no need
to fill this one in. Please feel free to paste this information in the PR
message. Thank you!
------------------------------
In schemes/gravity_wave_drag/gravity_wave_drag_moving_mountain.F90
<#448 (comment)>
:
> + !------------------------------------------
+ ! Expose contents of "P" coords1D structure
+ ! for improved clarity
+ !------------------------------------------
+ pmid(:ncol,:) = p%mid(:ncol,:)
+ delp(:ncol,:) = p%del(:ncol,:)
+
call gw_movmtn_src(ncol, pver, &
- u, v, ttend_dp(:ncol, :), xpwp_clubb(:ncol, :), &
- vorticity(:ncol, :), zm, alpha_gw_movmtn, &
+ u, v, ttend_dp(:ncol,:), xpwp_clubb(:ncol,:), &
+ vorticity(:ncol,:), zm, pmid, delp, prect(:ncol), alpha_gw_movmtn, &
I think it would be more straightforward to just use the p% structure
directly as it's only used once here to avoid a copy and its performance
hit:
⬇️ Suggested change
- !------------------------------------------
- ! Expose contents of "P" coords1D structure
- ! for improved clarity
- !------------------------------------------
- pmid(:ncol,:) = p%mid(:ncol,:)
- delp(:ncol,:) = p%del(:ncol,:)
-
- call gw_movmtn_src(ncol, pver, &
- u, v, ttend_dp(:ncol, :), xpwp_clubb(:ncol, :), &
- vorticity(:ncol, :), zm, alpha_gw_movmtn, &
- u, v, ttend_dp(:ncol,:), xpwp_clubb(:ncol,:), &
- vorticity(:ncol,:), zm, pmid, delp, prect(:ncol), alpha_gw_movmtn, &
+ call gw_movmtn_src(ncol, pver, &
+ u, v, ttend_dp(:ncol,:), xpwp_clubb(:ncol,:), &
+ vorticity(:ncol,:), zm, p%mid(:ncol,:), p%del(:ncol,:), prect(:ncol), alpha_gw_movmtn, &
------------------------------
In schemes/gravity_wave_drag/gravity_wave_drag_moving_mountain.F90
<#448 (comment)>
:
> - ! Midpoint zonal/meridional winds.
- real(kind_phys), intent(in) :: u(:, :), v(:, :), vorticity(:, :)
- ! Heating rate due to convection.
- real(kind_phys), intent(in) :: netdt(:, :) !from deep scheme
- ! Higher order flux from ShCu/PBL.
- real(kind_phys), intent(in) :: xpwp_shcu(:, :)
- ! Midpoint altitudes.
- real(kind_phys), intent(in) :: zm(:, :)
- ! tunable parameter controlling proportion of PBL momentum flux emitted as GW
- real(kind_phys), intent(in) :: alpha_gw_movmtn
-
- ! Indices of top gravity wave source level and lowest level where wind
- ! tendencies are allowed.
Here and below can I ask if there was a particular reason to remove the
comments describing the arguments? I can see an argument (pun not intended)
for removing the trivial ones like for u, v but I found the ones for
xpwp_shcu, etc. quite useful. Thanks!
------------------------------
In schemes/gravity_wave_drag/gravity_wave_drag_moving_mountain.F90
<#448 (comment)>
:
> !----------------------------------------------------------------------
- ! Initialize tau array
+ ! Initialise
I see your Claude likes British spelling... :-)
I don't have a feeling towards using one versus the other so I'm happy to
keep this as is if intended.
—
Reply to this email directly, view it on GitHub
<#448?email_source=notifications&email_token=ACGLMTTD5JKKJ66XMOCKXVL5QVM2PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMZQHAYDAMRQHEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-5308002093>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGLMTTZAWSXJMA4A6X2UEL5QVM2PAVCNFSNUABFKJSXA33TNF2G64TZHMZDEOJUHEYDIMJQHNEXG43VMU5TKNJXGE4TEMJWGQ22C5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ACGLMTR7TFMY36MGUC23L7L5QVM2PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMZQHAYDAMRQHEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/ACGLMTX72TWZO2E4CDNY7JL5QVM2PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMZQHAYDAMRQHEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
<ESCOMP/atmospheric_physics/pull/448/review/5308002093 <(530)%20800-2093>@
github.com>
--
*My working day may not be your working day. Please do not feel obliged to
reply to this message outside of your normal working hours.*
|
Collaborator
Author
|
Hi Haipeng,
I've looked over the comments in gravity_wave_drag_moving_mountain.F90 and
I am satisfied all of the changes in the new code are good ones. There has
actually been quite a bit more than two weeks of work here, and I have been
changing the comments as I went.
-Julio
…On Thu, Sep 24, 2026 at 11:47 AM Haipeng Lin ***@***.***> wrote:
***@***.**** commented on this pull request.
Some initial questions and comments.
Overall this looks good but I wonder about the significant comment removal
from the original GW moving mountain code in this PR. @JulioTBacmeister
<https://github.com/JulioTBacmeister>, can I ask if this was intentional
cleanup (e.g., the comments are stale, or not needed)? The amount of
removed comments appear nontrivial to me, so I wanted to confirm if it is
intentional or Claude took the decision to clean things up.
------------------------------
In doc/ChangeLog
<#448 (comment)>
:
> @@ -1,5 +1,61 @@
===============================================================
+Tag name: TBD (assigned at merge)
A note that doc/ChangeLog in atmos_phys is deprecated so there's no need
to fill this one in. Please feel free to paste this information in the PR
message. Thank you!
------------------------------
In schemes/gravity_wave_drag/gravity_wave_drag_moving_mountain.F90
<#448 (comment)>
:
> + !------------------------------------------
+ ! Expose contents of "P" coords1D structure
+ ! for improved clarity
+ !------------------------------------------
+ pmid(:ncol,:) = p%mid(:ncol,:)
+ delp(:ncol,:) = p%del(:ncol,:)
+
call gw_movmtn_src(ncol, pver, &
- u, v, ttend_dp(:ncol, :), xpwp_clubb(:ncol, :), &
- vorticity(:ncol, :), zm, alpha_gw_movmtn, &
+ u, v, ttend_dp(:ncol,:), xpwp_clubb(:ncol,:), &
+ vorticity(:ncol,:), zm, pmid, delp, prect(:ncol), alpha_gw_movmtn, &
I think it would be more straightforward to just use the p% structure
directly as it's only used once here to avoid a copy and its performance
hit:
⬇️ Suggested change
- !------------------------------------------
- ! Expose contents of "P" coords1D structure
- ! for improved clarity
- !------------------------------------------
- pmid(:ncol,:) = p%mid(:ncol,:)
- delp(:ncol,:) = p%del(:ncol,:)
-
- call gw_movmtn_src(ncol, pver, &
- u, v, ttend_dp(:ncol, :), xpwp_clubb(:ncol, :), &
- vorticity(:ncol, :), zm, alpha_gw_movmtn, &
- u, v, ttend_dp(:ncol,:), xpwp_clubb(:ncol,:), &
- vorticity(:ncol,:), zm, pmid, delp, prect(:ncol), alpha_gw_movmtn, &
+ call gw_movmtn_src(ncol, pver, &
+ u, v, ttend_dp(:ncol,:), xpwp_clubb(:ncol,:), &
+ vorticity(:ncol,:), zm, p%mid(:ncol,:), p%del(:ncol,:), prect(:ncol), alpha_gw_movmtn, &
------------------------------
In schemes/gravity_wave_drag/gravity_wave_drag_moving_mountain.F90
<#448 (comment)>
:
> - ! Midpoint zonal/meridional winds.
- real(kind_phys), intent(in) :: u(:, :), v(:, :), vorticity(:, :)
- ! Heating rate due to convection.
- real(kind_phys), intent(in) :: netdt(:, :) !from deep scheme
- ! Higher order flux from ShCu/PBL.
- real(kind_phys), intent(in) :: xpwp_shcu(:, :)
- ! Midpoint altitudes.
- real(kind_phys), intent(in) :: zm(:, :)
- ! tunable parameter controlling proportion of PBL momentum flux emitted as GW
- real(kind_phys), intent(in) :: alpha_gw_movmtn
-
- ! Indices of top gravity wave source level and lowest level where wind
- ! tendencies are allowed.
Here and below can I ask if there was a particular reason to remove the
comments describing the arguments? I can see an argument (pun not intended)
for removing the trivial ones like for u, v but I found the ones for
xpwp_shcu, etc. quite useful. Thanks!
------------------------------
In schemes/gravity_wave_drag/gravity_wave_drag_moving_mountain.F90
<#448 (comment)>
:
> !----------------------------------------------------------------------
- ! Initialize tau array
+ ! Initialise
I see your Claude likes British spelling... :-)
I don't have a feeling towards using one versus the other so I'm happy to
keep this as is if intended.
—
Reply to this email directly, view it on GitHub
<#448?email_source=notifications&email_token=ACGLMTTD5JKKJ66XMOCKXVL5QVM2PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMZQHAYDAMRQHEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-5308002093>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGLMTTZAWSXJMA4A6X2UEL5QVM2PAVCNFSNUABFKJSXA33TNF2G64TZHMZDEOJUHEYDIMJQHNEXG43VMU5TKNJXGE4TEMJWGQ22C5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ACGLMTR7TFMY36MGUC23L7L5QVM2PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMZQHAYDAMRQHEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/ACGLMTX72TWZO2E4CDNY7JL5QVM2PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMZQHAYDAMRQHEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
<ESCOMP/atmospheric_physics/pull/448/review/5308002093 <(530)%20800-2093>@
github.com>
--
*My working day may not be your working day. Please do not feel obliged to
reply to this message outside of your normal working hours.*
|
Per reviewer feedback on PR ESCOMP#448: the 3-way merge reconciliation had collapsed gw_movmtn_src's argument and local-variable declarations into a denser one-per-line style, which dropped the descriptive comments that were present upstream. Restores those comments (wording preserved where the underlying quantity is unchanged), and adds comments for the new arguments introduced by this PR (pmid, delp, tilt, p_steer, p_launch). No executable code changed; verified via case.build (QPC7, ne3pg3_ne3pg3_mt232, casper): 0 warnings/errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… GWD - New schemes/utilities/compute_total_precipitation_rate: prect = prec_dp + prec_sh + prec_str, providing lwe_precipitation_rate_at_surface for gravity_wave_drag_moving_mountain. Called from both CAM (gw_drag_cam) and CAM-SIMA so the summation order is identical. - Run it before gravity_wave_drag_moving_mountain in suite_cam7 and suite_gw_cam7_se. - Fix units of momentum_flux_source_for_moving_mountain_gravity_wave_drag in the moving mountain SIMA diagnostics (m2 s-2 -> Pa) to match the scheme metadata; capgen otherwise errors on the unsupported unit conversion. Assisted-by: claude-opus:5.5
jimmielin
marked this pull request as ready for review
September 30, 2026 19:50
jimmielin
approved these changes
Sep 30, 2026
peverwhee
self-requested a review
September 30, 2026 20:01
- Add TILT_MOVMTN, PSTEER_MOVMTN, PLAUNCH_MOVMTN and MMTAUE/W/S/N, matching the fields added to gw_drag_cam. - Rename UCELL/VCELL_MOVMTN to USTEER/VSTEER_MOVMTN, as in CAM. Assisted-by: claude-opus:5.5
peverwhee
requested changes
Sep 30, 2026
peverwhee
left a comment
peverwhee
left a comment
Collaborator
There was a problem hiding this comment.
a couple small cleanup requests
- Drop unused locals tau0 and hdmm_idx from gw_movmtn_src. - Compute CS directly instead of through the CS1 temporary (same expression, bit-for-bit). - Drop the unused z_steer/z_launch outputs of vorticity_centroid_levels. Assisted-by: claude-opus:5.5
peverwhee
requested changes
Sep 30, 2026
peverwhee
left a comment
peverwhee
left a comment
Collaborator
There was a problem hiding this comment.
sorry two more little things
Both are set for every source_type, not only 3-5. Assisted-by: claude-opus:5.5
peverwhee
approved these changes
Oct 1, 2026
jimmielin
changed the title
Collaborator
|
dry run of regression tests passed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds flow-dependent tilt/precipitation source options to the moving-mountain
gravity wave drag scheme, plus new cardinal-direction Reynolds-stress
diagnostics. This is a companion PR to ESCOMP/CAM#1684, which
adds the corresponding
MMTAUE/W/S/Nhistory outputs and namelistdocumentation.
movmtn_sourcevalues 3-5 (tilt-layer-mean; tilt+precip6-parameter fit; tilt+precip PySR cx17 fit). Existing values 1
(vorticity, default) and 2 (PBL momentum flux) are unchanged.
intent(out):taucd_west/east/south/north(directional Reynolds stresses fromgw_common'scalc_taucd), plustilt,p_steer,p_launch.prectinput, used only by the new precip-dependent source options.movmtn_sourceto 3/4/5; default behavior and answers are unchanged.AI involvement disclosure
This PR was produced through heavy, sustained collaboration with Claude
(Anthropic's Claude Code), across every stage: reconciling two weeks of
local development against current
mainvia 3-way merge, diagnosing andfixing a merge-introduced compile error via an actual test build, adding
the new diagnostic outputs, and drafting this PR/ChangeLog text. All
changes were reviewed and directed by the human author (Julio Bacmeister)
throughout, but the mechanical and drafting work was substantially done by
Claude, not just lightly assisted.
Test plan
ne3pg3_ne3pg3_mt232grid, caspermachine): 0 warnings/errors in the modified files.
aux_camregression suite (derecho/intel, derecho/nvhpc, izumi/nag,izumi/gnu) has not yet been run against this branch — opening as
draft pending that.
movmtn_sourcedefaults to 1.🤖 Generated with Claude Code