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

cam6_4_023: SCAM-SE feature addition plus bugfixes and some refactoring. #958

Merged
merged 36 commits into from
Aug 26, 2024

Conversation

jtruesdal
Copy link
Collaborator

@jtruesdal jtruesdal commented Jan 10, 2024

This update includes some refactoring of SCAM, a few bugfixes, and adding the capability to use spectral elements dycore to do vertical transport in the column. The SE feature addition follows the E3SM implementation where a complete coarse resolution (ne3np4) of the SE dycore is initialized but only a single element is run through vertical transport with all element subcolumns being copies of the single column chosen by scmlat, scmlon.

Like the Eulerian version, SCAM-SE also has a bit for bit test to validate an exact run through the same physics as the full 3d model. Because SCAM updates the solution using a slightly different order of operations, the bfb capability is tested by making a special diagnostic run of CAM where the 3d model derives the phys/dyn tendency each time step and then recalculates the prognostic solution using the derived tendencies and SCAM's prognostic equation. This new solution (which is less precise (roundoff) due to the change in order of operations) is substituted for the full 3d solution at each time step of the model run. The substitution of the roundoff state in the 3d run allows SCAM to reproduce (BFB) each time step using the captured tendencies in the cam iop history file.

The SCAM-SE vertical advection skips the horizontal step and derives the floating level tendency based on the IOP prescribed vertical velocity. The floating levels are subsequently remapped at the end of the vertically Lagrangian dynamics step.

This PR also includes an update to .gitmodules for cice and CDEPS.
- update cice to fix scam regression failure
- update cdeps to fix CDEPS regression test build failures

Closes #957
Closes #853
Closes #742

Copy link

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

Hi John - thank you for your work on this, it's clear that you've put a lot of time and effort into it.
There are a lot of changes using user mods or test mods that should be done in config_compsets.xml - I know there are several ways to do this - user mods are the worst in my opinion, please consider changing. I also strongly feel that this PR should include a change to shr_scam_mod.F90 to remove the requirement for MCT.

Externals.cfg Outdated
@@ -45,7 +45,7 @@ required = True
[share]
tag = share1.0.17
protocol = git
repo_url = https://github.com/ESCOMP/CESM_share
repo_url = https://github.com/jtruesdal/CESM_share

Choose a reason for hiding this comment

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

Why did you change to a fork if you otherwise have no mods here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'll double check this and get rid of it. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tag name should be share1.0.17_scamdev. I had a few mods for shr_scam_mod that turned up in testing for this PR. If you can help with the mods for getting rid of MCT I'm happy to include them, test and create the PR to CESM_share. In the meantime I'll fix the tag reference above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jedwards4b forgot to 'at' you on the previous question. Repeating here:

Tag name should be share1.0.17_scamdev. I had a few mods for shr_scam_mod that turned up in testing for this PR. If you can help with the mods for getting rid of MCT I'm happy to include them, test and create the PR to CESM_share. In the meantime I'll fix the tag reference above.

Choose a reason for hiding this comment

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

Sorry I haven't looked into it yet, I'll try to do it tomorrow.

./xmlchange STOP_OPTION=nhours
./xmlchange STOP_N=479

if [ `./xmlquery --value CAM_DYCORE` == 'se' ]; then

Choose a reason for hiding this comment

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

Why put this in a shell command instead of in config_compsets.xml?

Copy link
Collaborator Author

@jtruesdal jtruesdal Jan 16, 2024

Choose a reason for hiding this comment

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

I've put together an alternate version that we can compare to using usermods. I am testing now and it will be ready soon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See conversation below with Cheryl.

module apply_iop_forcing_mod

use shr_kind_mod, only:r8 => shr_kind_r8, i8 => shr_kind_i8
use pmgrid

Choose a reason for hiding this comment

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

Please add only clause.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@cacraigucar
Copy link
Collaborator

Hi John - thank you for your work on this, it's clear that you've put a lot of time and effort into it. There are a lot of changes using user mods or test mods that should be done in config_compsets.xml - I know there are several ways to do this - user mods are the worst in my opinion, please consider changing. I also strongly feel that this PR should include a change to shr_scam_mod.F90 to remove the requirement for MCT.

I'd like to discuss the suggestion from @jedwards4b about putting the individual SCAM setups in config_compsets.xml. I lean towards the way it is currently done, where things which apply to all compsets of a certain "flavor" are put into config_compsets.xml and the individual details are in use_cases in order to not make config_compsets.xml bloated with individual cases. This also makes it very clear to a user what is being set in a particular SCAM setup (and provides the template for them creating their own). Perhaps @jedwards4b can expand on why config_compset.sml is the better choice over user_mods? It is possible I am interpreting his suggestion incorrectly as well, so more details would be welcome.

@jedwards4b
Copy link

@cacraigucar I have to disagree, I think that user mods really obfuscate things - but I have a meeting with John at 2 today to look at this and maybe what we should do is create another branch so that we can compare and contrast the two approaches.

@cacraigucar
Copy link
Collaborator

@cacraigucar I have to disagree, I think that user mods really obfuscate things - but I have a meeting with John at 2 today to look at this and maybe what we should do is create another branch so that we can compare and contrast the two approaches.

I'm getting ready to start reviewing this PR and was curious about any proposed changes that might be made due to your meeting yesterday? @jedwards4b @jtruesdal

@cacraigucar cacraigucar self-requested a review January 12, 2024 17:18
Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

Submitting the items I have found so far - have not completed the review by any means

Externals.cfg Outdated Show resolved Hide resolved
bld/namelist_files/namelist_defaults_cam.xml Show resolved Hide resolved
bld/namelist_files/namelist_defaults_cam.xml Outdated Show resolved Hide resolved
bld/namelist_files/namelist_defaults_cam.xml Outdated Show resolved Hide resolved
bld/namelist_files/namelist_defaults_cam.xml Outdated Show resolved Hide resolved
src/control/ncdio_atm.F90 Outdated Show resolved Hide resolved
src/control/ncdio_atm.F90 Outdated Show resolved Hide resolved
src/control/scamMod.F90 Outdated Show resolved Hide resolved
src/control/scamMod.F90 Outdated Show resolved Hide resolved
src/control/scamMod.F90 Outdated Show resolved Hide resolved
@jtruesdal
Copy link
Collaborator Author

@cacraigucar` I have to disagree, I think that user mods really obfuscate things - but I have a meeting with John at 2 today to look at this and maybe what we should do is create another branch so that we can compare and contrast the two approaches.

I'm getting ready to start reviewing this PR and was curious about any proposed changes that might be made due to your meeting yesterday? @jedwards4b @jtruesdal

We did meet and as it turns out I already had a mostly complete set of mods for moving to a compset version of the different IOPs. I wanted to see what the effect of pushing the IOP configuration under cime_config would be. So I created a new branch with them to compare to the current way of doing things.

https://github.com/jtruesdal/CAM-1/compare/b567e51..jtruesdal:CAM-1:scam_dev_exp_w_compsets?expand=1

The mods add a cam6 configuration option for each IOP. I also added an alias for each IOP although we could keep one main IOP alias and allow users to switch between the various IOPS by specifying the correct longname. Having the IOP configure options also allow the user to run any IOP case using any of the other configuration options available in the longname. We no longer need the usermods scam directories and they have been deleted in the branch. Although if someone wants to add their own IOP then they will most likely do so through the usermods directory, so from the user perspective nothing has changed in that respect.

Whether they should be a configured via compsets is still an open question. Although they are not configuring for new physics parameterizations, one of the changes being added in this commit is the ability to modify the CLUBB parameterization to be more consistent with the type of forcing represented by the IOP. After looking at the comparison, they don't seem to overly complicate the xml files and these options make it easier to use the framework default configurations for each IOP via the compset longnames instead of having to individually specify them via usermods. I agree with Jim that this is a better way to setup the standard IOP's while still allowing usermods for user defined IOPs or user_nl_cam to tweak the setup like we do with other compsets.

@jtruesdal
Copy link
Collaborator Author

@jedwards4b wrote:

I also strongly feel that this PR should include a change to shr_scam_mod.F90 to remove the requirement for MCT.

I'm happy to include this change but after looking at the code in question I don't know how to make the proper changes for NUOPC. Would you be able to help or provide a set of mods to use? I'm happy to test along with the rest of the PR.

src/physics/cam/check_energy.F90 Outdated Show resolved Hide resolved
src/physics/cam/check_energy.F90 Show resolved Hide resolved
src/infrastructure/phys_grid.F90 Outdated Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Outdated Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Outdated Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Outdated Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Outdated Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Outdated Show resolved Hide resolved
src/dynamics/eul/iop.F90 Outdated Show resolved Hide resolved
@jedwards4b
Copy link

@jtruesdal looking at this this morning, I don't see any tests of scam on the ne grid listed in the cam/cime_config/testdefs/testlist_cam.xml Can you please add at least one test and also tell me what compset and resolution I should try.

@jedwards4b
Copy link

I tried test SMS.ne3pg3_ne3pg3_mg37.FSCAM.derecho_intel and ran into missing input data:

Model cam missing file drydep_srf_file = '/glade/campaign/cesm/cesmdata/inputdata/atm/cam/chem/trop_mam/atmsrf_ne3np4.pg3_221214.nc'
Trying to download file: 'atm/cam/chem/trop_mam/atmsrf_ne3np4.pg3_221214.nc' to path '/glade/campaign/cesm/cesmdata/inputdata/atm/cam/chem/trop_mam/atmsrf_ne3np4.pg3_221214.nc' using SVN protocol.
svn export failed with output:  and errput svn: E170000: URL 'https://svn-ccsm-inputdata.cgd.ucar.edu/trunk/inputdata/atm/cam/chem/trop_mam/atmsrf_ne3np4.pg3_221214.nc' doesn't exist


@jedwards4b
Copy link

@jtruesdal I tried to run test SCT_D_Ln7.T42_T42_mg17.QPC6.derecho_intel.cam-scm_prep_c6 and SCT_D_P36_Ln7.T42_T42_mg17.QPC6.derecho_intel.cam-scm_prep_c6 and both fail with an mpi error. Can you point me to an SCT test that runs on derecho - or izulme.

@jedwards4b
Copy link

I also tried these tests on izumi and both fail. I'm looking into the issue but where are the baselines and what tests have you run?

@jtruesdal
Copy link
Collaborator Author

Thanks Jim. You can hold for now, I see my SCT test failed also and am looking into it.

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Great work! I realize it looks like there are a ton of change requests, but most of them should either be trivial, or are just general questions. Thanks!

bld/build-namelist Outdated Show resolved Hide resolved
bld/config_files/definition.xml Outdated Show resolved Hide resolved
bld/namelist_files/namelist_defaults_cam.xml Show resolved Hide resolved
bld/namelist_files/namelist_defaults_cam.xml Show resolved Hide resolved
bld/namelist_files/namelist_defaults_cam.xml Show resolved Hide resolved
src/control/scamMod.F90 Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Outdated Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Outdated Show resolved Hide resolved
src/dynamics/se/se_single_column_mod.F90 Outdated Show resolved Hide resolved
src/dynamics/eul/iop.F90 Outdated Show resolved Hide resolved
Externals.cfg Outdated Show resolved Hide resolved
@jtruesdal
Copy link
Collaborator Author

@cacraigucar I removed the bfb testing line and updated to cam6_4_006 for diagnostic testing.

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Almost there! Just a few final minor requests and one missed request from last time.

bld/namelist_files/namelist_defaults_cam.xml Show resolved Hide resolved
src/control/scamMod.F90 Outdated Show resolved Hide resolved
src/dynamics/se/advect_tend.F90 Outdated Show resolved Hide resolved
src/physics/cam/check_energy.F90 Show resolved Hide resolved
@jtruesdal
Copy link
Collaborator Author

It's turning out to be more difficult to write heat_glob as a global and will require pretty extensive changes in addition to restructuring due to a circular dependency. I would ask to leave it as is for this commit. It's currently consistent with the way global values were treated by SCAM using the EUL dycore. The global value is written at each lat/lon point and the entire surface field is written to the CAM IOP file. (This is why an ncol array is created and written out). The SCAM physgrid definition only contains 1 column and the appropriate global value is read from the user chosen column.

@jtruesdal jtruesdal requested a review from nusbaume August 6, 2024 03:38
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Everything looks great to me now. Thanks @jtruesdal!

@cacraigucar cacraigucar changed the title SCAM-SE feature addition plus bugfixes and some refactoring. cam6_4_023: SCAM-SE feature addition plus bugfixes and some refactoring. Aug 20, 2024
@jtruesdal
Copy link
Collaborator Author

@jtruesdal I tried to run test SCT_D_Ln7.T42_T42_mg17.QPC6.derecho_intel.cam-scm_prep_c6 and SCT_D_P36_Ln7.T42_T42_mg17.QPC6.derecho_intel.cam-scm_prep_c6 and both fail with an mpi error. Can you point me to an SCT test that runs on derecho - or izulme.

@jedwards4b I see that I forgot to address your question about failing SCT tests. I did correct the error and the tests are running now on both Derecho and Izumi. The is a baseline difference due to a bug fix for SCAM. I'm asking for a re-review to clear this issue so I can proceed with the PR. If you would like to look at the Derecho and Izumi tests they are:
/scratch/cluster/jet/aux_cam_gnu_20240826004033
/scratch/cluster/jet/aux_cam_nag_20240826004050
/glade/derecho/scratch/jet/aux_cam_intel_20240826003035

Copy link

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

Thanks John!

@jtruesdal jtruesdal merged commit 6e32d03 into ESCOMP:cam_development Aug 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

4 participants