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

GW-RT global_control.nml modifications #2425

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

dpsarmie
Copy link
Collaborator

@dpsarmie dpsarmie commented Sep 3, 2024

Commit Queue Requirements:

  • Fill out all sections of this template.
  • All sub component pull requests have been reviewed by their code managers.
  • Run the full Intel+GNU RT suite (compared to current baselines) on either Hera/Derecho/Hercules
  • Commit 'test_changes.list' from previous step

There is now an option to allow default_vars.sh to set variables that are dependent on DT_ATMOS and ATMRES. If you wish to use a non-default value for either variable and have the script set all dependent variables automatically (ex. DT_INNER, CDMBGWD, namsfc filenames, etc.), then DT_ATMOS and ATMRES must be defined before calling the export functions.

If the user does not define the variable, then the defaults of C96 (ATMRES) and 1800 (DT_ATMOS for atm only) or 720 (DT_ATMOS for cpld cases) will be set.

Example:
Your RT uses C48 resolution and you want a timestep of 720:
In your test configuration:

export ATMRES=C48
export DT_ATMOS=720
export_fv3
export_ugwpv1
export_cpl
<the rest of the config>

This will now be able to use the user defined timestep and resolution to populate the other variables needed to run this case. As always, the order of the export function calls should go export_fv3, export_ugwpv1, then export_cpl. You can remove export function calls as needed for your particular case.

You are still able to use the old methodology of defining all variables manually. This feature just allows users to streamline that process.

Description:

This PR will modify the global_control.nml to add variables that are present in the Global Workflow scripts but not currently present in the RTs. Some variables will also be changed from constants to variable placeholders. This will allow Global Workflow to use the global_control.nml as a template for their operations.
Variable values that were hard-coded in global_control.nml.IN were replaced with an @ tag and the values were moved to default_vars.sh.

This PR is now also going to include a fix for #2443. This will add baseline changes that are unrelated to the initial global_control.nml modifications or the sfs configuration fix.
Default values for DT_ATMOS and ATMRES are now being set as intended. If the user defines either of these variables before the export_ function calls, then the appropriate dependent variables will be auto-configured. This removes the need to redefine these variables in the test configs.

This PR now includes a feature mentioned in #2470. The checks have been added to run_test.sh.
Changes in run_test.sh now have checks for the fast and slow coupling frequencies and a check on the CICE to ATM timesteps.

Commit Message:

* UFSWM - Modify global_control.nml to add compatibility with Global Workflow

Priority:

  • Normal

Git Tracking

UFSWM:

Sub component Pull Requests:

  • None

UFSWM Blocking Dependencies:

  • None

Changes

Regression Test Changes (Please commit test_changes.list):

  • cpld_control_p8_mixedmode intel
  • cpld_control_gfsv17 intel
  • cpld_control_gfsv17_iau intel
  • cpld_restart_gfsv17 intel
  • cpld_mpi_gfsv17 intel
  • cpld_control_sfs intel
  • cpld_debug_gfsv17 intel
  • cpld_control_p8 intel
  • cpld_control_p8.v2.sfc intel
  • cpld_restart_p8 intel
  • cpld_control_qr_p8 intel
  • cpld_restart_qr_p8 intel
  • cpld_2threads_p8 intel
  • cpld_decomp_p8 intel
  • cpld_mpi_p8 intel
  • cpld_control_ciceC_p8 intel
  • cpld_control_c192_p8 intel
  • cpld_restart_c192_p8 intel
  • cpld_bmark_p8 intel
  • cpld_restart_bmark_p8 intel
  • cpld_s2sa_p8 intel
  • cpld_control_noaero_p8 intel
  • cpld_control_nowave_noaero_p8 intel
  • cpld_debug_p8 intel
  • cpld_debug_noaero_p8 intel
  • cpld_control_c48 intel
  • cpld_warmstart_c48 intel
  • cpld_restart_c48 intel
  • cpld_control_p8_faster intel
  • cpld_control_pdlib_p8 intel
  • cpld_restart_pdlib_p8 intel
  • cpld_mpi_pdlib_p8 intel
  • cpld_debug_pdlib_p8 intel
  • atm_ds2s_docn_dice intel
  • cpld_control_nowave_noaero_p8 gnu
  • cpld_control_pdlib_p8 gnu
  • cpld_debug_pdlib_p8 gnu

Input data Changes:

  • None.

Library Changes/Upgrades:

  • No Updates

Testing Log:

  • RDHPCS
    • Hera
    • Orion
    • Hercules
    • Jet
    • Gaea
    • Derecho
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
  • opnReqTest (complete task if unnecessary)

dpsarmie and others added 2 commits August 30, 2024 23:49
This adds new options to the namelist that are used and
dynamic in the global workflow. This is to prepare the RT namelist
for the RT-GW bridge PR.
@dpsarmie dpsarmie marked this pull request as ready for review September 6, 2024 18:01
This commit changes a couple of the parameters in
cpld_control_sfs to bring it in line with the Global
Workflow parameter values.
@dpsarmie dpsarmie added the Baseline Updates Current baselines will be updated. label Sep 16, 2024
@dpsarmie dpsarmie marked this pull request as draft September 17, 2024 15:49
dpsarmie and others added 2 commits September 20, 2024 08:07
This commit addresses the issue in default_vars.sh where
ATMRES is reset in the export_cpl() call to C96.
Default values will now only be set if the resolutions have
not been previously set.
@NickSzapiro-NOAA
Copy link
Collaborator

@dpsarmie I also hit a problem with DT_ATMOS. For gfsv17 tests for example:

  1. export_fv3 sets a default DT_ATMOS
  2. export_cpl sets a different default DT_ATMOS that is used in export_{cice,cmeps,...} for DT_CICE, coupling_interval_fast_sec,..
  3. Then export_ugwpv1 sets a different DT_ATMOS based on ATMRES
  4. But cice, cmeps,... never see changes from step 3

Maybe export_cpl should not touch DT_ATMOS either.

It may also be better in these tests to call export_fv3, then export_ugwpv1, then export_cpl so atmosphere is configured properly before setting other components

@dpsarmie dpsarmie marked this pull request as ready for review September 21, 2024 23:45
@dpsarmie
Copy link
Collaborator Author

dpsarmie commented Sep 23, 2024

Thanks @NickSzapiro-NOAA for bringing that up. I'll have to do some more testing to see what makes sense. I think the simplest solution would be to make the atm only and cpld default DT_ATMOS the same, but I want to make sure that the runtime isn't greatly affected for the atm only cases.
If not, then there needs to be a straightforward way to differentiate between the possible combinations and what needs to be defined.
Although I agree that the ugwpv1 call should come before the cpld call to resolve your issue 4.

@dpsarmie dpsarmie marked this pull request as draft September 23, 2024 13:38
@NickSzapiro-NOAA
Copy link
Collaborator

I think it is better if DT_ATMOS is thought of as input to export_cpl (and not modified in export_cpl). The issue I'm focusing on is (coupling) timesteps for gfsv17 test, where ufs.configure is the same before and after 2363:

# CMEPS warm run sequence
runSeq::
@3600
   MED med_phases_prep_wav_avg
   MED med_phases_prep_ocn_avg
   MED -> WAV :remapMethod=redist
   MED -> OCN :remapMethod=redist
   WAV
   OCN
   @720
     MED med_phases_prep_atm
     MED med_phases_prep_ice
     MED -> ATM :remapMethod=redist
     MED -> ICE :remapMethod=redist
     ATM
     ICE
     ATM -> MED :remapMethod=redist
     MED med_phases_post_atm
     ICE -> MED :remapMethod=redist
     MED med_phases_post_ice
     MED med_phases_ocnalb_run
     MED med_phases_prep_ocn_accum
     MED med_phases_prep_wav_accum
   @
   OCN -> MED :remapMethod=redist
   WAV -> MED :remapMethod=redist
   MED med_phases_post_ocn
   MED med_phases_post_wav
   MED med_phases_restart_write
@
::

But, dt_atmos in model_configure is 360s in current develop vs 720s before 2363. Was it intended for 2363 to reduce timestep for this test? Either way, now FV3<-->CMEPS no longer couple every ATM timestep, ATM timestep is different than ICE timestep,... I'm not sure this is intentional and may need hot fix if not intended.

@dpsarmie
Copy link
Collaborator Author

dpsarmie commented Sep 23, 2024

Ok I see what you mean.

#2363 was intended to change the timesteps to have the more optimized set of parameter values to allow for more stability at longer timesteps: NOAA-EMC/global-workflow#2574

I'm going to move forward with your idea of having DT_ATMOS set in export_fv3 and changed in export_ugwpv1 (if needed) before calling export_cpl.

That should allow DT_CICE and coupling_interval_fast_sec to be set correctly when the other export calls are run from within export_cpl.

Line 1302 will also be removed in default_vars.sh since DT_INNER will have been set by then.

Let me know if that logic makes sense. That should resolve the issues of not having the timing in sync between the models, correct?

@NickSzapiro-NOAA
Copy link
Collaborator

That makes sense to me. Maybe can ask Denise to review solution too

@dpsarmie
Copy link
Collaborator Author

dpsarmie commented Oct 8, 2024

Latest changes do the following:

To fix the timing issue it CMEPS, default_vars.sh has been reworked. If you export DT_ATMOS and/or ATMRES before the export_fv3 (and other export functions), then all dependent variables will be auto-populated. If these variables are not set, then defaults will be set. You can still manually set these parameters after the function calls but you need to make sure that all variables that depend on DT_ATMOS and ATMRES are also set unless you intended to do so otherwise.

Either way, the timing with CMEPS should be functioning as intended.

@dpsarmie dpsarmie marked this pull request as ready for review October 8, 2024 12:49
@NickSzapiro-NOAA
Copy link
Collaborator

@dpsarmie In the PR description, can you add how the logic/flow in default_vars works and should be used?

@dpsarmie
Copy link
Collaborator Author

dpsarmie commented Oct 8, 2024

@NickSzapiro-NOAA I added a snippet to the description on how it should be used. I'm actually currently working on a more detailed document for how to use the new features, so let me know if anything is unclear or if anything else should be added to the description for the PR.

@DeniseWorthen
Copy link
Collaborator

@dpsarmie I'm getting confused about where the B4B differences are arising. It seems you're combining what is a bug-fix (the issue w/ dt_atmos), with updates of various namelist settings to match GW. Both of these tasks create non-B4B changes, is that right? Should we really be combining a bug fix w/ what is actually a new feature (consistency w/ G-W)?

@dpsarmie
Copy link
Collaborator Author

@DeniseWorthen Yes, the changes to the namelist and the associated changes to default_vars (just adding variables) were changes that were still B4B comparable.
There was a discussion a few weeks ago with EPIC to roll these other bug fixes into the PR since they are touching related files.

I've opened individual issues for documentation purposes since it is getting a bit complex. I'll add something to the description to try and detail the changes and which issue was addressed by which change in the code. There won't be anything else added to this PR.

@DeniseWorthen
Copy link
Collaborator

@dpsarmie Thanks for adding the information. Did either

Variable values that were hard-coded in global_control.nml.IN were replaced with an @ tag and the values were moved to default_vars.sh.
Another issue will also be merged into this PR. The cpld_control_sfs configuration will be changed to bring it in-line with the GW configuration. cpld_control_sfs settings were changed in the test configuration file.

change baselines?

I'm confused because the next change is described with This will add additional baseline changes. (emphasis added).

If either of the first two items changed baselines, then I believe it is poor CM practice to combine issues which both change a set of tests. Rolling them together makes it much harder to un-roll/debug in the future, if required.

@dpsarmie
Copy link
Collaborator Author

@DeniseWorthen Yes, the cpld_control_sfs test underwent two baseline changes. The GW settings change (second item under Description) and the DT_ATMOS fix (third item) had two distinct changes in that baseline.

I can untangle those easily and create an independent PR so there's only one baseline change per PR.

Comment on lines +911 to +934
export ISEED_SKEB=0
export SKEB_TAU=21600,
export SKEB_LSCALE=500000,
export SKEBNORM=1,
export SKEB_NPASS=30,
export SKEB_VDOF=5,
export ISEED_SHUM=1,
export SHUM_TAU=21600,
export SHUM_LSCALE=500000,
export ISEED_SPPT=20210325000103,20210325000104,20210325000105,20210325000106,20210325000107
export SPPT_TAU=2.16E4,2.592E5,2.592E6,7.776E6,3.1536E7
export SPPT_LSCALE=500.E3,1000.E3,2000.E3,2000.E3,2000.E3
export SPPT_LOGIT=.true.,
export SPPT_SFCLIMIT=.true.,
export USE_ZMTNBLCK=.true.
export PBL_TAPER=0,0,0,0.125,0.25,0.5,0.75
export OCNSPPT=0.8,0.4,0.2,0.08,0.04
export OCNSPPT_LSCALE=500.E3,1000.E3,2000.E3,2000.E3,2000.E3
export OCNSPPT_TAU=2.16E4,2.592E5,2.592E6,7.776E6,3.1536E7
export ISEED_OCNSPPT=20210325000108,20210325000109,20210325000110,20210325000111,20210325000112
export EPBL=0.8,0.4,0.2,0.08,0.04
export EPBL_LSCALE=500.E3,1000.E3,2000.E3,2000.E3,2000.E3
export EPBL_TAU=2.16E4,2.592E5,2.592E6,7.776E6,3.1536E7
export ISEED_EPBL=20210325000113,20210325000114,20210325000115,20210325000116,20210325000117
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think SKEB, SPPT, SHUM are missing default values (like EPBL).

If I understand, are you wanting these to default to what is being used in GEFS prototype? Maybe @pjpegion can confirm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I'm was setting defaults based off of GW. There are some missing entries that are not here that will be added.
I'm scheduling a meeting soon with GW to talk about some discrepancies in the RT and GW namelist and what they would prefer.
The missing defaults are here on a branch I created for the GW PR: https://github.com/dpsarmie/global-workflow/blob/feature/GW-RT_GlobalNML/global_control.nml.IN#L393

Comment on lines +93 to +109
# UGWP1
export GWD_OPT=2
export KNOB_UGWP_NSLOPE=1
export DO_GSL_DRAG_LS_BL=.true.
export DO_GSL_DRAG_SS=.true.
export DO_UGWP_V1_OROG_ONLY=.false.
export DO_UGWP_V0_NST_ONLY=.false.
export LDIAG_UGWP=.false.

export DO_GSL_DRAG_TOFD=.false.
export CDMBWD=${CDMBWD_c96}

# UGWD
export DO_UGWP_V0=.true.
export DO_UGWP_V1=.false.
export DO_GSL_DRAG_LS_BL=.false.
export KNOB_UGWP_VERSION=0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this different than the cpld_control_nowave_noaero_p8 test it depends on?

Comment on lines 1181 to +1182
export DT_INNER=${DT_ATMOS}

export default_dt_atmos=0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is DT_INNER always equal to DT_ATMOS?

Why is the value of default_dt_atmos changing here? Is it used somewhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the test is running the ugwpv1, then yes DT_INNER will be set to DT_ATMOS. If we set DT_INNER to half of DT_ATMOS (which is what GW is doing), then a handful of tests run over the allotted wall clock time. I did this to keep the RTs under the wall clock, but I did test the RTs using the GW parameters that were discussed here: #2363 to make sure that they were stable on our end.

default_dt_atmos changes here to make sure that it does not change in export_cpl(). I'm allowing export_cpl() to change DT_ATMOS for now to keep functionality with ugwpv0. When the time comes to deprecate version 0, then L1403 - 1407 should be removed.

@dpsarmie
Copy link
Collaborator Author

@dpsarmie I also hit a problem with DT_ATMOS. For gfsv17 tests for example:

  1. export_fv3 sets a default DT_ATMOS
  2. export_cpl sets a different default DT_ATMOS that is used in export_{cice,cmeps,...} for DT_CICE, coupling_interval_fast_sec,..
  3. Then export_ugwpv1 sets a different DT_ATMOS based on ATMRES
  4. But cice, cmeps,... never see changes from step 3

Maybe export_cpl should not touch DT_ATMOS either.

It may also be better in these tests to call export_fv3, then export_ugwpv1, then export_cpl so atmosphere is configured properly before setting other components

This was resolved by adding a default_dt_atmos variable. If DT_ATMOS is not defined before the export_fv3 is called, then it will be set to its default value. Since the coupled cases and the ugwpv1 cases all have their unique DT_ATMOS settings, they will also change DT_ATMOS if it was set to default.

The new ordering of the function calls should be export_fv3, export_ugwpv1, export_cpl. If default_dt_atmos is 1, export_fv3 will set DT_ATMOS to the default value (default for uncoupled cases), export_ugwpv1 will then set DT_ATMOS to the compatible value for gwpv1 based on grid resolution. The final export_cpl call will not change DT_ATMOS and then cice and cmeps will get the final DT_ATMOS value.

All other model configuration combinations should also work. ATM only (export_fv3), coupled using gwpv0 (export_fv3, export_cpl), ATM gwpv1 only, (export_fv3, export_ugwpv1), and coupled gwpv1 (export_fv3, export_ugwpv1, export_cpl) are all working as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Baseline Updates Current baselines will be updated.
Projects
None yet
5 participants