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

Add choice of computed or constant ocean GM gravity wave speed (AMOC PR 1/6) #4835

Merged
merged 6 commits into from
Apr 6, 2022

Conversation

mark-petersen
Copy link
Contributor

@mark-petersen mark-petersen commented Mar 16, 2022

Currently the internal gravity wave speed (first baroclinic mode) in the ocean GM parameterization is a constant, previously named config_GM_constant_gravWaveSpeed but changed in this PR to config_GM_constant_bclModeSpeed. This remains the default, but this PR adds a new flag, config_GM_minBclModeSpeed_method: If 'constant' (the default) then use config_GM_constant_bclModeSpeed. If 'computed' then compute the gravity wave speed at every edge at every time step using the Brunt-Vaisala frequency. This functionality was tested by the AMOC focus team in 2021 on the branch amoc-experimental-branch.

[NML]
[BFB]

@mark-petersen mark-petersen added BFB PR leaves answers BFB NML labels Mar 16, 2022
@mark-petersen mark-petersen force-pushed the mark-petersen/ocn/gm-min-phase-speed branch from 96a2036 to f2253c1 Compare March 16, 2022 21:35
Copy link
Contributor

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

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

@mark-petersen this is a great idea to implement, thanks for the PR. I had some confusion though when going through it. It's possible I'm not understanding something

components/mpas-ocean/src/shared/mpas_ocn_gm.F Outdated Show resolved Hide resolved
components/mpas-ocean/src/shared/mpas_ocn_gm.F Outdated Show resolved Hide resolved
@vanroekel
Copy link
Contributor

@mark-petersen I realize I got ,confused by your PR description and it led to incorrect comments on my review. Your title suggests you are implementing a spatially variable computation of c and this made me thing of cGMphaseSpeed, but this PR is really just to do a spatially variable c_min. So a few suggestions

  1. change config_GM_gravwavespeed to something like config_GM_gravwavespeed_min
  2. same for the gm_constant_gravWaveSpeed variable
  3. Can the PR description be changed as well without too much trouble?
  4. Although more likely the subject of a separate PR gravWaveSpeed is not a correct description. It is actually the first baroclinic mode, it would be nice to drop the grav or change that to something like bcl, but again, I don't think that needs to be here.

I think with those first 3 clarifications this PR will be good to go from my view.

c_min = gm_minBclModeSpeed_constant + gm_minBclModeSpeed_compute_on*max(0.01_RKIND,sumN2/ltSum*(0.5*(lt1+lt2)))

! Compute the speed of the first baroclinic mode from the Brunt-Vaisala frequency.
cGMphaseSpeed(iEdge) = max(c_min, &
sumN2/(config_GM_spatially_variable_baroclinic_mode*3.141592_RKIND))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vanroekel I changed the flag names to

    config_GM_constant_bclModeSpeed = 0.3
    config_GM_minBclModeSpeed_method = 'constant'

The only substantial change is the line above, but I use fixed coefficient values set on init to avoid if statements inside the loop.

@mark-petersen
Copy link
Contributor Author

mark-petersen commented Mar 20, 2022

I rebased this on today's master. It now passes nightly suite bfb with both gnu and intel against master. Also passes:

SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.anvil_intel
SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.anvil_gnu

@stephenprice, this is ready for the next cryo test in the sequence. You only needed to change this flag to

config_GM_minBclModeSpeed_method = 'computed'

to invoke the changes in this PR.

@milenaveneziani
Copy link
Contributor

My 5-year test with ECwISC30to60E2r1, this branch, and the setting config_GM_minBclModeSpeed_method = 'computed' did not produce any visible difference with a previous 5-year test that used a baseline configuration.

MPAS-Analysis for this branch:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.milena/20220322.CRYO1850.ne30pg2_ECwISC30to60E2r1.chrysalis.CryoBranchGMphase/Years1-5/
MPAS-Analysis for the baseline:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.milena/20220301.CRYO1850.ne30pg2_ECwISC30to60E2r1.chrysalis.CryoBranchBaseline/Years1-5/

Copy link
Contributor

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

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

@mark-petersen this looks great. This clears up the misunderstanding I had on this PR in my initial comments. I'm approving by visual inspection.

@vanroekel
Copy link
Contributor

@mark-petersen but to verify this PR is BFB right? It looked it to me

@mark-petersen
Copy link
Contributor Author

Yes, if config_GM_gravwavespeed='const', the default, then it is bfb with previous, and that was shown in the testing. Thanks @vanroekel

@mark-petersen
Copy link
Contributor Author

@jonbob this is ready to merge when the repo is open. Thanks.

@jonbob
Copy link
Contributor

jonbob commented Mar 30, 2022

@mark-petersen - the PR description is no longer current with the code changes? I don't see those variable names in the code

@jonbob
Copy link
Contributor

jonbob commented Apr 4, 2022

@mark-petersen - I updated the bld files with output from the auto-generating scripts. There were some changes to the namelist definition file, if you want to make sure you agree.

@mark-petersen
Copy link
Contributor Author

Thanks @jonbob, I updated the description. Your updated scripts are correct.

@jonbob
Copy link
Contributor

jonbob commented Apr 4, 2022

@mark-petersen - this PR is failing SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel. I'll look at it more this evening

@mark-petersen mark-petersen changed the title add choice of computed or constant ocean GM gravity wave speed Add choice of computed or constant ocean GM gravity wave speed (AMOC PR 1/6) Apr 5, 2022
@mark-petersen
Copy link
Contributor Author

mark-petersen commented Apr 5, 2022

@jonbob this now passes SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel. I initialized thread-private scalars outside the threaded loop.

@jonbob
Copy link
Contributor

jonbob commented Apr 5, 2022

That fix makes sense, @mark-petersen. I'll re-test and aim to merge today

jonbob added a commit that referenced this pull request Apr 5, 2022
Add choice of computed or constant ocean GM gravity wave speed

Currently the internal gravity wave speed (first baroclinic mode) in the
ocean GM parameterization is a constant, previously named
config_GM_constant_gravWaveSpeed but changed in this PR to
config_GM_constant_bclModeSpeed. This remains the default, but this PR
adds a new flag, config_GM_minBclModeSpeed_method: If 'constant' (the
default) then use config_GM_constant_bclModeSpeed. If 'computed' then
compute the gravity wave speed at every edge at every time step using
the Brunt-Vaisala frequency. This functionality was tested by the AMOC
focus team in 2021 on the branch amoc-experimental-branch.

[NML]
[BFB]
@jonbob
Copy link
Contributor

jonbob commented Apr 5, 2022

passed:

  • ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel
  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel

merged to next

@jonbob jonbob merged commit 565da94 into master Apr 6, 2022
@jonbob
Copy link
Contributor

jonbob commented Apr 6, 2022

merged to master and expected NML DIFFs blessed

@jonbob jonbob deleted the mark-petersen/ocn/gm-min-phase-speed branch April 6, 2022 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB mpas-ocean NML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants