Port SE dycore to CAMDEN #135

nusbaume · 2021-06-16T19:02:52Z

Adds the SE dycore to CAMDEN from CAM, including bug fixes that impacted the use of the dycore in CAMDEN.

Fixes #44
Fixes #71
Fixes #76
Fixes #78
Fixes #86
Fixes #136

Addresses #91
Addresses #123
Addresses #150

Tests ran:

Ran an FKESSLER compset at ne5_ne5_mg37 and ne5pg2_ne5pg2_mg37 (for CSLAM) resolution for five time steps for both NTASKS=1 and NTASKS=2 pe layouts. Performed smoke tests with Intel, PGI, and NAG compilers, and ensured bit-for-bit reproducibility with CAM when using Intel. I also ran a regular null dycore kessler run to make sure I didn't break anything.

It should be noted that in order to have bit-for-bit agreement the physics interfaces in CAM needed to be modified to more closely match CAMDEN (which was easier than modifying CAMDEN given that many of the interfaces were auto-generated). Those modifications (assuming you are using cam6_3_019) can be found on cheyenne here:

/glade/u/home/nusbaume/CAMDEN_backup_files/SE-dycore/bit-for-bit/cam_kessler

Finally, most of the lines of code being added by this PR are straight copies from CAM, particularly for files in the dyn/se/dycore directory. However, I did make minor modifications to most of those files, so it might be easiest to review this PR by diffing these files with the same files from CAM (tag = cam6_3_019). Of course if you really want to review the entire SE dycore then I certainly won't stop you.

Anyways, thanks in advance for your comments and suggestions, and have a great day!

…eration.

…ycore.

…structure.

…d_init' in SE dycore.

… numbers.

(Please note that this commit excludes a required Kessler physics and/or registry temperature tendency variable name change, which should be brought in separately. Until that change is implemented either registry.xml or kessler_update.meta must be modified manually.)

…ne namelist default value.

…ate calls.

…fix namelist bug.

…process (Github issue #140).

gold2718

Wow, this turned out to be a huge project!
Lots of great work.
I have a bunch of questions, comments and some change requests.

cime_config/cam_config.py

test/unit/cam_config_unit_tests.py

cime_config/namelist_definition_cam.xml

…ions.

nusbaume · 2021-08-26T17:30:37Z

@gold2718 this PR is ready to be reviewed again. Thanks!

cime_config/cam_config.py

cime_config/namelist_definition_cam.xml

cime_config/cam_config.py

gold2718

This is my review based on 0f9daf1. My earlier comments were published prematurely by GitHub (I think it happened because I commented on a conversation while my review was in progress). Please add these changes to the previous ones and then ask for a re-review. Sorry for the confusion.

src/data/generate_registry_data.py

src/data/physconst.F90

src/dynamics/utils/dyn_thermo.F90

gold2718 · 2021-09-06T02:58:57Z

src/dynamics/utils/dyn_thermo.F90

+      real(kind_phys) :: dp_dry_phys(i0:i1,j0:j1,k0:k1)
+      real(kind_phys) :: dp_phys(i0:i1,j0:j1,k0:k1)
+      real(kind_phys), allocatable :: ps(:,:)
+      real(kind_phys), allocatable :: ptop


Does a scalar real need to be allocatable?

In this case, yes, because it is an optional input that needs to be converted, and so having it allocated (or not) is how the receiving subroutine knows whether the converted value is "present".

Nice technique but should probably document that intent in the code because I forgot and now I recall having the discussion a few weeks ago.

Please add this documentation.

Comment has been added explaining why the scalar real variable is allocatable.

src/dynamics/utils/dyn_thermo.F90

nusbaume · 2021-09-21T14:59:35Z

@gold2718 This code is ready for review again. Thanks!

cime_config/cam_config.py

gold2718 · 2021-09-22T16:59:06Z

cime_config/cam_config.py

@@ -27,7 +29,7 @@
 from cam_autogen import generate_init_routines


-# Determine regular rexpression type  (for later usage in Config_string)
+# Determine regular rexpression type (for later usage in check_string_val)


You might be able to replace these checks with:

isinstance(<x>, re.Pattern)

which is more pythonic. Python seems to support this by documenting the re.Pattern class (https://docs.python.org/3/library/re.html#re-objects).

That re.Pattern object doesn't appear to be available in Python 3.6 or earlier. We could restrict CAMDEN to Python 3.7 or later, but that would put us out-of-sync with CIME in terms of python requirements. I am already planning to create an issue describing the need to update the python version and listing all of the various associated code changes, so if you want I can include this change in that list.

Also, in terms of the python version, I have found that for a complete python version restriction to work we'll need to update the CIME tag to ensure that all shebangs are python3. Otherwise, at least on Izumi, there is at least one section in the CIME run sequence where the python version will automatically use the default python version (which is 2.7). I went ahead and added the code change needed for CAMDEN to still run on Izumi, which is also included in the python 3.7 modification list, but at this point I would like to avoid updating the CIME tag for this PR, given that we are so close already and most of these changes aren't really required for the SE dycore port. Let me know if you disagree, however. For now you can look at issue #156 for more info.

Agreed on #156 but I would say that we are overdue for a CIME upgrade so please make sure this is prioritized.

cime_config/cam_config.py

src/data/generate_registry_data.py

src/data/physconst.F90

src/data/registry.xml

gold2718

This is Xeno's review of a012973.
A few more tweaks please.

src/dynamics/se/dyn_comp.F90

src/dynamics/se/dp_coupling.F90

nusbaume · 2021-09-24T22:45:39Z

@gold2718 this PR is ready for re-review again. Thanks!

gold2718

A couple of suggested changes but generally okay.

gold2718 · 2021-09-26T02:29:03Z

cime_config/cam_config.py

+            # Check that all tuple elements are integers:
+            for valid_val in valid_vals:
+                if valid_val is not None and not isinstance(valid_val, int):
+                    emsg = ("ERROR:  Valid value, '{}', for variable '{}', must be "
+                            "either None or an integer.  Currently it is '{}'.")
+                    raise CamConfigTypeError(emsg.format(valid_val, name, type(valid_val)))
+                # End if
+            # End for


Consider checking all values:

Suggested change

# Check that all tuple elements are integers:

for valid_val in valid_vals:

if valid_val is not None and not isinstance(valid_val, int):

emsg = ("ERROR: Valid value, '{}', for variable '{}', must be "

"either None or an integer. Currently it is '{}'.")

raise CamConfigTypeError(emsg.format(valid_val, name, type(valid_val)))

# End if

# End for

# Check that all tuple elements are integers:

emsg = ""

for valid_val in valid_vals:

if valid_val is not None and not isinstance(valid_val, int):

emsg += ("ERROR: Valid value, '{}', for variable '{}', must be "

"either None or an integer. Currently it is '{}'.\n")

# End if

# End for

if emsg:

raise CamConfigTypeError(emsg.format(valid_val, name, type(valid_val)))

# end if

I swear that someday I'll remember to do this before the code review! All values are now checked.

gold2718 · 2021-09-26T02:29:31Z

cime_config/cam_config.py

+            # Check that all tuple elements are integers:
+            for valid_val in valid_vals:
+                if valid_val is not None and not isinstance(valid_val, int):
+                    emsg = ("ERROR:  Valid value, '{}', for variable '{}', must be "
+                            "either None or an integer.  Currently it is '{}'.")
+                    raise CamConfigTypeError(emsg.format(valid_val, name, type(valid_val)))
+                # End if
+            # End for


I think it would be nicer if you did "check that all tuple elements are integers".

Suggested change

# Check that all tuple elements are integers:

for valid_val in valid_vals:

if valid_val is not None and not isinstance(valid_val, int):

emsg = ("ERROR: Valid value, '{}', for variable '{}', must be "

"either None or an integer. Currently it is '{}'.")

raise CamConfigTypeError(emsg.format(valid_val, name, type(valid_val)))

# End if

# End for

emsg = ""

# Check that all tuple elements are integers:

for valid_val in valid_vals:

if valid_val is not None and not isinstance(valid_val, int):

emsg += ("ERROR: Valid value, '{}', for variable '{}', must be "

"either None or an integer. Currently it is '{}'.\n")

# End if

# End for

if emsg:

raise CamConfigTypeError(emsg.format(valid_val, name, type(valid_val)))

# end if

All elements are now checked (even if a bad value is found).

nusbaume added 27 commits June 15, 2021 20:38

Add modifications needed for SE dycore configuration and namelist gen…

6172d21

…eration.

Remove extra public copy of 'iMap' (Github issue #78).

47ecaa7

Add 'air_composition_nl' namelist group, which is needed for the SE d…

afa9da8

…ycore.

Add SE dycore source code, modified to work with new host model infra…

570e648

…structure.

Update SE dycore code to head of ESCOMP/CAM cam_development branch.

7e6a42d

Perform miscellaneous code cleanup.

1884f96

Add ability to set CPP definitions. Also fix python and meta file bugs.

6a85704

Add cam_development updates to physics_grid, and implement 'model_gri…

e194d66

…d_init' in SE dycore.

Make modifications necessary for model to compile with SE dycore.

a34d9ac

Remove pmgrid.F90 and replace with vert_coord module.

31f8be0

Allow SE dycore to use run-time configured vertical levels and tracer…

d87fb55

… numbers.

Add stat checks to all 'allocate' statements in SE dycore.

1f820f5

Fix runtime namelist errors for SE dycore.

b813ba2

Apply code fixes for SE dycore initialization.

42ea4f9

Remove temporary dyn_init debugging code.

9810fde

Fix bug in physconst, and add extra mark_as_init calls.

a64e7c2

Remove mistakenly added debugging code in registry generator.

cf8c726

Fix 'allow_abrev' python version issue, and use 'csne' to set the se_…

a7e54d8

…ne namelist default value.

Add bug fixes needed to run with PGI and NAG compilers.

77521d2

Remove FVM_TRACERS CPP def, and perform some code clean-up.

4420c65

Ensure the proper kind for physconst calls, and add extra check_alloc…

db48d38

…ate calls.

Add final SE-dycore check_allocate calls, move ps to phys_state, and …

11d7eb2

…fix namelist bug.

Fix issues caused by rebase and perform minor clean-up.

f1fa068

Fix namelist bugs found using CSLAM.

6c16f1f

Fix unit tests.

cc05977

Fix issues with null dycore simulation.

60086d8

nusbaume added enhancement New feature or request bug-fix This PR was created to fix a specific bug. labels Jun 16, 2021

nusbaume requested a review from gold2718 June 16, 2021 19:02

cacraigucar approved these changes Jul 16, 2021

View reviewed changes

Make buildnml and buildlib use python3 if called directly or as a sub…

436245f

…process (Github issue #140).

gold2718 requested changes Aug 2, 2021

View reviewed changes

nusbaume added 6 commits August 16, 2021 10:27

Create new 'dynconst' and 'dyn_thermo' modules to handle kind convers…

7b96279

…ions.

Merge remote-tracking branch 'upstream/development' into se_dycore

25cca87

Fix parse_config_opts doctests.

c265f43

Replace CPP ifdefs with runtime options DDT object.

2c5891f

Address review comments and suggestions.

1e56860

Fix pylint errors.

0f9daf1

nusbaume requested a review from gold2718 August 26, 2021 17:30

gold2718 reviewed Sep 3, 2021

View reviewed changes

gold2718 requested changes Sep 6, 2021

View reviewed changes

This was referenced Sep 16, 2021

Need to find more efficient method for selecting chosen physics suite from user_nl_cam #154

Closed

CAMDEN should initialize tendencies #123

Open

nusbaume added 4 commits September 20, 2021 20:45

Address second round of review comments and suggestions.

4354b9d

Fix pylint errors.

9887dd9

Fix doctest failure in Python 3.5

82bd95d

Modify registry to work with null dycore.

a012973

nusbaume requested a review from gold2718 September 21, 2021 14:58

gold2718 reviewed Sep 23, 2021

View reviewed changes

gold2718 requested changes Sep 23, 2021

View reviewed changes

Address third round of review comments and suggestions.

61182fb

nusbaume requested a review from gold2718 September 24, 2021 22:45

gold2718 approved these changes Sep 26, 2021

View reviewed changes

Address final round of review comments and suggestions.

d66cb95

nusbaume merged commit 14bb4a2 into ESCOMP:development Sep 27, 2021

nusbaume deleted the se_dycore branch October 8, 2021 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port SE dycore to CAMDEN #135

Port SE dycore to CAMDEN #135

nusbaume commented Jun 16, 2021 •

edited

Loading

gold2718 left a comment

nusbaume commented Aug 26, 2021

gold2718 left a comment

gold2718 Sep 6, 2021

nusbaume Sep 21, 2021

gold2718 Sep 23, 2021

gold2718 Sep 26, 2021

nusbaume Sep 27, 2021

nusbaume commented Sep 21, 2021

gold2718 Sep 22, 2021

nusbaume Sep 24, 2021

gold2718 Sep 26, 2021

gold2718 left a comment

nusbaume commented Sep 24, 2021

gold2718 left a comment

gold2718 Sep 26, 2021

nusbaume Sep 27, 2021 •

edited

Loading

gold2718 Sep 26, 2021

nusbaume Sep 27, 2021

Port SE dycore to CAMDEN #135

Port SE dycore to CAMDEN #135

Conversation

nusbaume commented Jun 16, 2021 • edited Loading

Tests ran:

gold2718 left a comment

Choose a reason for hiding this comment

nusbaume commented Aug 26, 2021

gold2718 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nusbaume commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gold2718 left a comment

Choose a reason for hiding this comment

nusbaume commented Sep 24, 2021

gold2718 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nusbaume Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nusbaume commented Jun 16, 2021 •

edited

Loading

nusbaume Sep 27, 2021 •

edited

Loading