Implementation of CCPP timestep_init and timestep_final phases #534

climbfuji · 2020-12-18T03:49:14Z

This PR contains the following changes:

move time vary physics from run to timestep_init
remove dependency of time vary physics on GFS DDTs
clean up o3 and h2o physics (use existing arrays instead of duplicating them and wasting memory)

Associated PRs:

ufs-community/ufs-weather-model#337
NOAA-EMC/fv3atm#217
#534
NCAR/ccpp-framework#344
NOAA-EMC/GFDL_atmos_cubed_sphere#47

For regression testing information, see ufs-community/ufs-weather-model#337.

…y on GFS DDTs

…mestep_init_final

…llowing the merge of NCAR master

…mestep_init_final

climbfuji · 2020-12-28T17:33:17Z

I am opening this PR for review. Although unlikely, I may have to pull in updates from NCAR master in case anything gets committed until it is the turn for this PR. But the code changes itself should be final.

tanyasmirnova

From all affected by this PR subroutines I am familiar with the gcycle.F90. Changes look good to me.

SMoorthi-emc · 2020-12-29T00:19:35Z

Dom,
I am not sure what changed and I don't see the updated code.
Besides, I am on leave until the beginning of next year. So, may be I should not be reviewing this PR I guess.
Moorthi

climbfuji · 2020-12-29T00:24:57Z

Dom,
I am not sure what changed and I don't see the updated code.
Besides, I am on leave until the beginning of next year. So, may be I should not be reviewing this PR I guess.
Moorthi

Absolutely not, I guess you shouldn't even be checking your emails ;-) No hurries at all, this is not yet a PR (just an issue noting the problem and suggesting a solution).

…3.{F90,meta}

… from physics/GFS_rrtmg_setup.{F90,meta}

…eta}

…f variabale 'rann'

…ep_init

…mestep_init_final

SMoorthi-emc

I have looked at the code, but cannot verify if everything is correct.
Nevertheless, I have to trust the CCPP team.

grantfirl · 2021-01-07T22:40:45Z

@climbfuji Maybe I should know this, but for purposes of reviewing, is there any documentation that explains the new timestep_init and timestep_final CCPP phases? What is possible to do within them, what data should they have access to, etc.?

climbfuji · 2021-01-07T22:50:12Z

@climbfuji Maybe I should know this, but for purposes of reviewing, is there any documentation that explains the new timestep_init and timestep_final CCPP phases? What is possible to do within them, what data should they have access to, etc.?

I was looking at the ccpp-framework issues and meeting minutes, there is no good description of those. Maybe @gold2718 wrote a proposal back when we discussed it first time. In our meeting minutes, it appears first and with limited information on 06/06/2019, then again 06/04/2020:

https://github.com/NCAR/ccpp-framework/wiki/CCPP-Framework-Meeting-Minutes-2019-06-06
https://github.com/NCAR/ccpp-framework/wiki/CCPP-Framework-Meeting-Minutes-2020-06-04

In short: timestep_init and timestep_final captures processes that setup or finalize the physics for the current timestep. This is exactly what is in the current time_vary physics group. Similar as for the init and finalize phases, the rule is that no external loops over blocked data structures are allowed, i.e. all columns of an MPI rank must be passed at once. Threading is allowed inside the timestep_init/timestep_final physics, though.

The restriction is for the same reason as for the init and final phases, namely that such steps likely involve reading data (updated tables etc) from disk, or MPI synchronization.

climbfuji · 2021-01-07T23:38:18Z

@SMoorthi-emc @grantfirl Thanks for reviewing. Note: So far, regression tests have passed on the following platforms against the existing baselines. This means that we can be fairly confident that the changes were made correctly. jet.intel is about to finish, same for cheyenne.intel. Denise is running the wcoss tests,

	modified:   tests/RegressionTests_cheyenne.gnu.log
	modified:   tests/RegressionTests_gaea.intel.log
	modified:   tests/RegressionTests_hera.gnu.log
	modified:   tests/RegressionTests_hera.intel.log
	modified:   tests/RegressionTests_orion.intel.log

climbfuji · 2021-01-08T17:36:10Z

@SMoorthi-emc @grantfirl Thanks for reviewing. Note: So far, regression tests have passed on the following platforms against the existing baselines. This means that we can be fairly confident that the changes were made correctly. jet.intel is about to finish, same for cheyenne.intel. Denise is running the wcoss tests,
	modified:   tests/RegressionTests_cheyenne.gnu.log
	modified:   tests/RegressionTests_gaea.intel.log
	modified:   tests/RegressionTests_hera.gnu.log
	modified:   tests/RegressionTests_hera.intel.log
	modified:   tests/RegressionTests_orion.intel.log

Update: all regression tests passed on all tier-1 platforms. That's a good indication that we got the code changes right.

grantfirl

Approved

grantfirl · 2021-01-08T19:42:57Z

physics/GFS_rad_time_vary.fv3.F90

+             if (kdt == 1) then
+               t_2delt  = t
+               t_1delt  = t
+               qv_2delt = qv


This looks nice and clean. Since the RTs passed, I guess there was no need for the max function? Or, is it because there are no RTs using ZC MP anymore?

No, we are running plenty of Zhao-Carr MP tests, including restart tests. They all passed. Means that the ICs already have a lower limit larger than that particular qmin (1.0E-10).

Will that always be the case that ICs have already enforced a minimum value prior to this point in the physics? In other words, was the reason for removing it because it truly isn't necessary to enforce a minimum at this point? For a PR that is not supposed to change the answer, did we just get lucky?

I think you are right. Let's fix this with your update of the corresponding SCM code. Thanks for finding this.

grantfirl · 2021-01-08T20:10:11Z

physics/GFS_phys_time_vary.fv3.F90

         ! Initialize CCPP error handling variables
         errmsg = ''
         errflg = 0

         if (is_initialized) return

-         nblks = size(Model%blksz)


I can't find an obvious place where this non-uniform block checking stuff would have gone. Is it magically handled in the CCPP framework now?

No, this was a consistency check that was added a while back when the CCPP static mode was developed. It is no longer needed, because the logic in fv3atm takes care of it for the remainder of the lifetime of ccpp_prebuild.py.

grantfirl · 2021-01-08T20:16:40Z

physics/GFS_phys_time_vary.fv3.F90

+         if (nscyc >  0) then
+           if (mod(kdt,nscyc) == 1) THEN
+             call gcycle (me, nthrds, nx, ny, isc, jsc, nsst, tile_num, nlunit,       &
+                 input_nml_file, lsoil, lsoil_lsm, kice, idate, ialb, isot, ivegsrc,  &


This had to be a ton of fun passing all of these in!

grantfirl · 2021-01-08T20:25:22Z

physics/GFS_rrtmg_setup.F90

@@ -523,12 +522,14 @@ subroutine radinit( si, NLAY, imp_physics, me )
 !> -# Set up control variables and external module variables in
 !!    module physparam
 #if 0
+      ! DH* WHAT IS THIS?


According to GitHub's blame function, this comment has existed since the original creation of the file, which was probably just copied from GFS_radiation_driver.F90 at the time. Maybe the comment was from Rusty Benson when he made IPD?

It most likely was something in from the radiation driver prior to the GFS physics cleanup that culminated in the IPD. I didn't knowingly change any science during the IPD creation.

grantfirl · 2021-01-08T20:27:07Z

physics/GFS_suite_interstitial.F90

@@ -16,15 +16,16 @@ end subroutine GFS_suite_interstitial_rad_reset_finalize
 !!
    subroutine GFS_suite_interstitial_rad_reset_run (Interstitial, Model, errmsg, errflg)

-      use GFS_typedefs, only: GFS_control_type,GFS_interstitial_type
+      use machine,      only: kind_phys


Why is kind_phys needed in this subroutine now?

This is an oversight, it's not needed.

grantfirl · 2021-01-08T20:28:41Z

I finally made my way through this -- really good work. I think that I understand it well enough to transfer the changes to SCM variants.

grantfirl · 2021-01-08T20:33:05Z

physics/GFS_phys_time_vary.fv3.F90

-
-        !--- determine if diagnostics buckets need to be cleared
-        sec_zero = nint(Model%fhzero*con_hr)
-        if (sec_zero >= nint(max(Model%fhswr,Model%fhlwr))) then


It's funny that in the latest SCM PR where I added support for diagnostics intervals other than every timestep, I removed the clear calls from the host in favor of what was in the physics_time_vary run. Now, it will need to be added back. Not a big deal, just funny timing.

This adds support for 32-bit physics to the FV3, based on prior work on the Neptune model. Co-authored-by: Dusan Jovic <[email protected]>

climbfuji added 3 commits December 17, 2020 20:33

Move time vary physics from run to timestep_init and remove dependenc…

e1e8c3f

…y on GFS DDTs

Merge branch 'master' of https://github.com/NCAR/ccpp-physics into ti…

eb1287f

…mestep_init_final

Updates to physics/GFS_phys_time_vary.fv3.* and physics/gcycle.F90 fo…

04ee898

…llowing the merge of NCAR master

Merge branch 'master' of https://github.com/NCAR/ccpp-physics into ti…

f8b5e52

…mestep_init_final

climbfuji marked this pull request as ready for review December 28, 2020 17:33

climbfuji requested review from grantfirl, JulieSchramm and llpcarson as code owners December 28, 2020 17:33

climbfuji requested review from SMoorthi-emc, tanyasmirnova and ligiabernardet December 28, 2020 17:37

tanyasmirnova approved these changes Dec 28, 2020

View reviewed changes

climbfuji mentioned this pull request Dec 29, 2020

Catch exception when parsing a Fortran file with syntax errors NCAR/ccpp-framework#231

Closed

climbfuji added 7 commits December 29, 2020 08:53

Bugfixes for OpenMP and Zhao-Carr MP in physics/GFS_phys_time_vary.fv…

08bd983

…3.{F90,meta}

Remove unused num_p2d = array_dimension_of_2d_arrays_for_microphysics…

7107b49

… from physics/GFS_rrtmg_setup.{F90,meta}

Remove reset of diagnostic buckets from GFS_suite_interstitial.{F90,m…

1eebfac

…eta}

Bugfix in physics/GFS_phys_time_vary.fv3.{F90,meta}, correct intent o…

e11fb71

…f variabale 'rann'

Bugfix in physics/GFS_rrtmgp_setup.{F90,meta}, rename _run to _timest…

f18c4ef

…ep_init

Merge branch 'master' of https://github.com/NCAR/ccpp-physics into ti…

e6c009d

…mestep_init_final

Merge branch 'master' of https://github.com/NCAR/ccpp-physics into ti…

bc22582

…mestep_init_final

SMoorthi-emc approved these changes Jan 7, 2021

View reviewed changes

grantfirl approved these changes Jan 8, 2021

View reviewed changes

climbfuji merged commit 9ae4cea into NCAR:master Jan 8, 2021

grantfirl reviewed Jan 8, 2021

View reviewed changes

grantfirl mentioned this pull request Jan 8, 2021

Reorganization of "time_vary" interstitials #549

Open

climbfuji deleted the timestep_init_final branch June 27, 2022 03:26

HelinWei-NOAA pushed a commit to HelinWei-NOAA/ccpp-physics that referenced this pull request Feb 26, 2023

32-bit physics with FV3_RAP (NCAR#534)

9743346

This adds support for 32-bit physics to the FV3, based on prior work on the Neptune model. Co-authored-by: Dusan Jovic <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of CCPP timestep_init and timestep_final phases #534

Implementation of CCPP timestep_init and timestep_final phases #534

climbfuji commented Dec 18, 2020 •

edited

Loading

climbfuji commented Dec 28, 2020

tanyasmirnova left a comment

SMoorthi-emc commented Dec 29, 2020

climbfuji commented Dec 29, 2020

SMoorthi-emc left a comment

grantfirl commented Jan 7, 2021

climbfuji commented Jan 7, 2021

climbfuji commented Jan 7, 2021

climbfuji commented Jan 8, 2021

grantfirl left a comment

grantfirl Jan 8, 2021

climbfuji Jan 8, 2021

grantfirl Jan 8, 2021

climbfuji Jan 9, 2021

grantfirl Jan 8, 2021

climbfuji Jan 8, 2021

grantfirl Jan 8, 2021

grantfirl Jan 8, 2021

bensonr Jan 8, 2021 •

edited

Loading

grantfirl Jan 8, 2021

climbfuji Jan 8, 2021

grantfirl commented Jan 8, 2021

grantfirl Jan 8, 2021

Implementation of CCPP timestep_init and timestep_final phases #534

Implementation of CCPP timestep_init and timestep_final phases #534

Conversation

climbfuji commented Dec 18, 2020 • edited Loading

climbfuji commented Dec 28, 2020

tanyasmirnova left a comment

Choose a reason for hiding this comment

SMoorthi-emc commented Dec 29, 2020

climbfuji commented Dec 29, 2020

SMoorthi-emc left a comment

Choose a reason for hiding this comment

grantfirl commented Jan 7, 2021

climbfuji commented Jan 7, 2021

climbfuji commented Jan 7, 2021

climbfuji commented Jan 8, 2021

grantfirl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bensonr Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantfirl commented Jan 8, 2021

Choose a reason for hiding this comment

climbfuji commented Dec 18, 2020 •

edited

Loading

bensonr Jan 8, 2021 •

edited

Loading