Merge stochastic physics (SPPT only) into GSL's develop branch #239
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,10 @@ | |
| description="The number of atmospheric layers"/> | ||
| <dim name="nVertLevelsP1" definition="nVertLevels+1" | ||
| description="The number of atmospheric levels, always one more than the number of layers"/> | ||
| <dim name="lon_for" definition="504" | ||
| description="The number of longitude points for the Gaussian grid"/> | ||
| <dim name="lat_leg" definition="248" | ||
| description="The number of latitude points for the Gaussian grid"/> | ||
|
Comment on lines
+41
to
+44
Collaborator
There was a problem hiding this comment. Without full comprehension of what these variables are used for, I think Copilot has some valid points here.
Collaborator
There was a problem hiding this comment. My browser quit on me so I don't know if this will be a duplicate comments or not... Without fully understanding what these variables are for, it seems Copilot has some good points here.
Collaborator
There was a problem hiding this comment. From @NingWang325, these arrays are only used for debugging. These fields aren't being used otherwise. They can be removed. |
||
|
|
||
| #ifdef DO_PHYSICS | ||
| <dim name="nMonths" definition="namelist:months" | ||
|
|
@@ -979,6 +983,8 @@ | |
| <var name="ke"/> | ||
| <var name="uReconstructZonal"/> | ||
| <var name="uReconstructMeridional"/> | ||
| <var name="stoch_pattern_sppt"/> | ||
| <var name="stoch_pattern_gg"/> | ||
|
|
||
| <!-- Begin Ertel PV diagnostics defined in diagnostics/Registry_pv.xml --> | ||
| <var name="ertel_pv"/> | ||
|
|
@@ -4162,6 +4168,11 @@ | |
| description="Total cell-centered meridional wind tendency from physics" | ||
| persistence="scratch" /> | ||
|
|
||
| <var name="stoch_pattern_sppt" type="real" dimensions="nVertLevels nCells Time" units="%" | ||
| description="Perturbation pattern for stochastic physics SPPT scheme" /> | ||
|
|
||
| <var name="stoch_pattern_gg" type="real" dimensions="lon_for lat_leg Time" units="%" | ||
| description="Perturbation pattern for stochastic physics, on Gaussian grid" /> | ||
| #ifdef DO_PHYSICS | ||
| <!-- ================================================================================================== --> | ||
| <!-- TENDENCIES FROM PARAMETERIZATION OF CLOUD MICROPHYSICS: --> | ||
|
|
@@ -4672,5 +4683,6 @@ | |
| #include "physics/Registry_noahmp.xml" | ||
| #include "physics/Registry_tempo.xml" | ||
| #include "physics/registry.chemistry.xml" | ||
| #include "stochastic_physics/Registry_stoch_physics.xml" | ||
| #endif | ||
| </registry> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ module atm_time_integration | |
| use mpas_atmphys_driver_microphysics | ||
| use mpas_atmphys_todynamics | ||
| use mpas_atmphys_utilities | ||
| use mpas_stochastic_physics, only : stochastic_physics_pattern_apply, dosppt | ||
| #endif | ||
|
|
||
| use mpas_atm_boundaries, only : nSpecZone, nRelaxZone, nBdyZone, mpas_atm_get_bdy_state, mpas_atm_get_bdy_tend ! regional_MPAS addition | ||
|
|
@@ -880,6 +881,8 @@ subroutine atm_srk3(domain, dt, itimestep, exchange_halo_group) | |
| #endif | ||
|
|
||
| real (kind=RKIND) :: time_dyn_step | ||
| character(len=32) :: tend_names(4) | ||
| integer :: ierr | ||
| logical, parameter :: debug = .false. | ||
|
|
||
|
|
||
|
|
@@ -1077,11 +1080,28 @@ subroutine atm_srk3(domain, dt, itimestep, exchange_halo_group) | |
| call mpas_timer_start('physics_get_tend') | ||
| rk_step = 1 | ||
| dynamics_substep = 1 | ||
|
|
||
| ! apply random perturbation pattern to the tendency | ||
|
Collaborator
There was a problem hiding this comment. tend_names being hardcoded here is a problem.
Collaborator
There was a problem hiding this comment. Why is this a problem? If there's a way to query which tendencies are being allocated, we could use that here and select (from that list), which to perturb based on the physics suite and stochastic method in question.
Collaborator
There was a problem hiding this comment. Two things. The tendencies being perturbed is hardcoded and not extensible. Also tend_names doesn't need to be set at each time-step before applying the pattern (i.e. this could be done during initialization once). For example, if you extend stochastic_physics_pattern_apply to perturb more tendencies, you will also need to modify the MPAS source code here. A solution would be to make Also, this allows another host model (e.g., UFS) to control which fields we want to perturb when using stochastic physucs within MPAS. We plan on having tend_names part of the nam_stochy namelist to allow this runtime felxibility. |
||
| tend_names(1) = "rucuten" | ||
| tend_names(2) = "rvcuten" | ||
| tend_names(3) = "rublten" | ||
| tend_names(4) = "rvblten" | ||
| if (dosppt(domain)) then | ||
| call stochastic_physics_pattern_apply(domain, 4, tend_names, ierr) | ||
| endif | ||
|
|
||
| call physics_get_tend( block, mesh, state, diag, tend, tend_physics, & | ||
| block % configs, rk_step, dynamics_substep, & | ||
| tend_ru_physics, tend_rtheta_physics, tend_rho_physics, & | ||
| exchange_halo_group ) | ||
| call mpas_timer_stop('physics_get_tend') | ||
|
|
||
| ! apply random perturbation pattern to the tendency | ||
| tend_names(1) = "tend_rtheta_physics" | ||
| ! tend_names(2) = "tend_rho_physics" | ||
| if (dosppt(domain)) then | ||
| call stochastic_physics_pattern_apply(domain, 1, tend_names, ierr) | ||
| endif | ||
| #else | ||
| #ifndef MPAS_CAM_DYCORE | ||
| ! | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ function atm_core_init(domain, startTimeStamp) result(ierr) | |
| use mpas_attlist, only : mpas_modify_att | ||
| use mpas_string_utils, only : mpas_string_replace | ||
| use mpas_atm_halos, only: atm_build_halo_groups, exchange_halo_group | ||
| use mpas_stochastic_physics, only : stochastic_physics_pattern_init, dosppt | ||
|
|
||
| implicit none | ||
|
|
||
|
|
@@ -301,6 +302,11 @@ function atm_core_init(domain, startTimeStamp) result(ierr) | |
| ! Prepare the dynamics for integration | ||
| ! | ||
| call mpas_atm_dynamics_init(domain) | ||
| ! | ||
| ! init stochastic pattern generation | ||
| if (dosppt(domain)) then | ||
| call stochastic_physics_pattern_init (domain, ierr) | ||
|
Collaborator
There was a problem hiding this comment. @gsketefian Should this die and report error if initialization fails?
Author
There was a problem hiding this comment. @dustinswales I would think it should at least write out a warning in the log file(s) if not die altogether, but I think @NingWang325 is best suited to answer this one. |
||
| endif | ||
|
Comment on lines
+305
to
+309
Collaborator
There was a problem hiding this comment. From @NingWang325: this code follows procedure from NOAA-PSL/stochastic_physics to ensure initialization is successful. For now, a message is passed to the log saying that stochastic physics could not be initialized. We need to stop the run in NOAA-PSL/stochastic_physics if initialization of any expect stochastic physics method fails. Check the "ierr" value and issue a stop command. |
||
|
|
||
| end function atm_core_init | ||
|
|
||
|
|
@@ -1014,6 +1020,7 @@ subroutine atm_do_timestep(domain, dt, itimestep) | |
| use mpas_atmphys_update | ||
| #endif | ||
| use mpas_atm_halos, only: exchange_halo_group | ||
| use mpas_stochastic_physics, only : stochastic_physics_pattern_adv, dosppt | ||
|
|
||
| implicit none | ||
|
|
||
|
|
@@ -1046,6 +1053,9 @@ subroutine atm_do_timestep(domain, dt, itimestep) | |
| endif | ||
| #endif | ||
|
|
||
| if (dosppt(domain)) then | ||
|
Collaborator
There was a problem hiding this comment. Could this call be moved out of atm_do_timestep into into atm_core_run? |
||
| call stochastic_physics_pattern_adv(domain, itimestep, ierr) | ||
| endif | ||
| call atm_timestep(domain, dt, currTime, itimestep, exchange_halo_group) | ||
|
|
||
| end subroutine atm_do_timestep | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. @gsketefian
Author
There was a problem hiding this comment. @dustinswales Which workflow are you referring to, and what do you mean by "beforehand"? I use Also, where is either From what you're describing, all this is already done somewhere. I'm happy to try out the existing capability and remove my changes. I'll tag @AndersJensen-NOAA too in case he has suggestions.
Collaborator
There was a problem hiding this comment. @gsketefian Building the TEMPO tables is not needed for stochastic physics and it's not currently part of the MPAS-A codebase. So why is this being added here?
Author
There was a problem hiding this comment. @dustinswales Whenever we (the DTC's stoch physics team) make changes to the stochastic physics code, the MPAS-Model tests I run are with both Thompson and TEMPO. I was under the impression TEMPO is the MP scheme we're planning to switch to, so I make sure to test that. If it's ok to test only with Thompson (or another scheme), that's ok with me. @JeffBeck-NOAA @clark-evans
Collaborator
There was a problem hiding this comment.
@gsketefian MPAS has never had this functionality.
Collaborator
There was a problem hiding this comment. There should be no reason to touch the core_atmosphere/physics Makefile in this PR.
Author
There was a problem hiding this comment. I will undo these changes to move the PR along. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,8 @@ core_physics_mmm: core_physics_init | |
| (cd physics_mmm; $(MAKE) -f Makefile.mpas all) | ||
|
|
||
| core_microphysics: core_physics_init core_physics_mmm | ||
|
Author
There was a problem hiding this comment. Changes in first line are to copy |
||
| (cd physics_noaa/TEMPO; $(MAKE) -f Makefile.mpas; cp tables/ccn_activate.bin ../../../../../) | ||
|
Author
There was a problem hiding this comment. The following new line builds TEMPO's Not sure why this wasn't originally included. Also, not sure if this work with all machines and compilers, but it works on Hera.
Author
There was a problem hiding this comment. @clark-evans Not sure if you're the right person to ask this. If not, please let me know who to ask (Anders?). I'm wondering why the original version of this Without this executable (named I will comment out this line for now so the CI tests can proceed, but it would be nice to understand why building
Collaborator
There was a problem hiding this comment. Hi @gsketefian -- I think this is handled in src/core_atmosphere/utils/Makefile, where directives for compiling build_tables_tempo are provided alongside those for build_tables. This is called when core_atmosphere is built. Tagging @AndersJensen-NOAA to correct me in case I'm wrong, though...
Author
There was a problem hiding this comment. @clark-evans @AndersJensen-NOAA As far as I can tell, that
Author
There was a problem hiding this comment. @clark-evans @AndersJensen-NOAA I talked to Anders today about this, and he pointed out that there is also a Makefile for gfortran ( Btw, the code in
Collaborator
There was a problem hiding this comment. @clark-evans, would you be OK with adding the TEMPO table executable to the "make" build as Gerard suggested?
Collaborator
There was a problem hiding this comment. @JeffBeck-NOAA following the discussion between @dustinswales and @gsketefian, I think we'll hold off on it for now to expedite this PR. It'd be a good separate PR to add afterwards, though.
Collaborator
There was a problem hiding this comment. @clark-evans, this is what I see from @dustinswales: "Building the TEMPO tables is not needed for stochastic physics and it's not currently part of the MPAS-A codebase. So why is this being added here?" While stochastic physics doesn't need the TEMPO tables, SPP will be in TEMPO at some point (probably in the next year). Any SPP regression tests would need to use TEMPO, but as stated, we can deal with that in the future when it's ready. |
||
| (cd physics_noaa/TEMPO; $(MAKE) -f Makefile.intel run_build_tables ; cp run_build_tables ../../../../../) | ||
|
|
||
| core_SMOKE: core_physics_init | ||
| (cd physics_noaa/SMOKE; cp ./MPAS/Makefile .; cp ./MPAS/mpas_smoke_wrapper.F90 .; $(MAKE) all) | ||
|
|
@@ -290,6 +291,7 @@ clean: | |
| ( cd physics_noahmp/src; $(MAKE) clean ) | ||
| ( cd physics_noahmp/utility; $(MAKE) clean ) | ||
| ( if [ -d physics_noaa/TEMPO ]; then cd physics_noaa/TEMPO; $(MAKE) -f Makefile.mpas clean; fi ) | ||
|
Author
There was a problem hiding this comment. The following is to clean the |
||
| ( if [ -d physics_noaa/TEMPO ]; then cd physics_noaa/TEMPO; $(MAKE) -f Makefile.intel clean; fi ) | ||
| ( if [ -d physics_noaa/UGWP ]; then cd physics_noaa/UGWP; $(MAKE) clean; fi ) | ||
| ( if [ -d physics_noaa/RUCLSM ]; then cd physics_noaa/RUCLSM; $(MAKE) -f MPAS/Makefile clean; fi ) | ||
| ( if [ -d physics_noaa/MYNN-EDMF ]; then cd physics_noaa/MYNN-EDMF; cp ./MPAS/Makefile .; $(MAKE) clean; fi ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -199,9 +199,9 @@ | |
| <!-- **************************************************************************************** --> | ||
|
|
||
| <streams> | ||
|
Author
There was a problem hiding this comment. This was necessary to avoid SMIOL errors apparently due to another stream named |
||
| <stream name="output_smoke" | ||
| type="output" | ||
| filename_template="history_smoke.$Y-$M-$D_$h.$m.$s.nc" | ||
|
Collaborator
There was a problem hiding this comment. @gsketefian This change will break the current rrfs-workflow.v2 (with smoke and dust) as discussed in #196 (comment) We need to discuss this and have an agreement before making this change. Thanks!
Author
There was a problem hiding this comment. I will undo these changes to move the PR along. I'll do some more tests to see if this is necessary. |
||
| output_interval="6:00:00" | ||
| runtime_format="separate_file"> | ||
| <var name="aero_emis_for_enhmix"/> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the new line here now uses
esmf_time_f91instead ofesmf_time_f90.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NingWang325 @clark-evans These intel mkl libraries are generating the following errors in the CI build tests:
With gnu compiler:
With intel compiler:
First, I thought this was due to gnu not recognizing these intel libraries, but it is also happening with the intel compiler. Do we need to change the modules when running the CI tests?
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear about the temporal order of these comments, but it appears that Copilot thinks the f91 should be f90 (immediately below) and that could solve the library link problem. Is f91 really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joeolson42 Turns out:
esmf_time_f91.esmf_time_f90turned intoesmf_time_f91.-I<dir>tells the compiler/preprocessor where to search for include/module files (.h, .mod, etc.). This is a compile-time flag, not a link-time flag, so it's not used in this command and can be removed.Since
-I<dir>is not used at all by this linker command, I removed it. I then was able to rebuild (both withmakeandcmake) without issues.