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

Add soil temperature and moisture IAU #222

Open
wants to merge 121 commits into
base: ufs/dev
Choose a base branch
from

Conversation

tsga
Copy link

@tsga tsga commented Aug 23, 2024

Add Incremental Analysis Update (IAU) to soil temperature and moisture in the NoahMP land model of the UWM's CCPP. Addresses issue: /issues/190

Regression tests for ATM only and coupled (S2S) configurations pass in hera.

Additional land-IAU specific test to be added: control_c48_intel_land_iau (based on the "control_c48_intel" test, with the only difference being the land IAU increment files added to the INPUT directory (sample increment files at ./scratch2/NCEPDEV/land/data/DA/soil_moisture/LandIAU/INC/C48/INC09/).

The input.nml is modified to add "land_iau_nml" (do_land_iau=.true. if executing with land IAU)

tsga and others added 30 commits March 16, 2024 16:56
…into feature/lnd_iau

* 'feature/lnd_iau' of https://github.com/tsga/ccpp-physics:
  correct the meta file
  correctly convert from flashes per five minutes to flashes per minute
  In physics/Interstitials/UFS_SCM_NEPTUNE/maximum_hourly_diagnostics.F90, scale lightning threat from flashes per 5 minutes to flashes per minute to match units in metadata
  In physics/Interstitials/UFS_SCM_NEPTUNE/maximum_hourly_diagnostics.meta: change units 'flashes 5 min-1' to 'flashes min-1' and update long name to make clear this is per 5 minutes
* tmp:
  fix arg_table_noahmpdrv_finalize
removed pointer attribute from "input_nml_file" declaration
remove commented out old lines
get rid unused var input_nml_length
physics/SFC_Models/Land/Noahmp/noahmpdrv.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.meta Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.meta Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.meta Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.meta Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.meta Outdated Show resolved Hide resolved
comment out unused number of freeze/thaw counters
make long names consistent with naming conventions

private

real(kind=kind_phys), allocatable :: wk3_stc(:, :, :, :), wk3_slc(:, :, :, :)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tsga @dustinswales @climbfuji I've made modifications to move the module-defined DDTs to GFS_typedefs (in the FV3 host). I haven't tested/pushed the changes yet. The last bit that could potentially cause a problem is with these variables wk3_stc, wk3_slc, wk3_slmsk. They are read in during the init phase and used throughout the simulation in the timestep_init phase, so, basically like LUTs, but they are functions of time and x,y,z. I suppose that it's possible that different instances of the CCPP could read in different files and therefore these variables would be different across instances and therefore would need to get moved to the host as well. @tsga What do you think? Do you think that somewhere down the line (in potentially a different host model), that these variables would be different for each instance?

Copy link
Author

@tsga tsga Oct 9, 2024

Choose a reason for hiding this comment

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

@grantfirl yes, different instances are likely to have different wk3_stc and wk3_slc (but not wk3_slmsk). Can these also be read by the host at init and passed through to noahmpdrv by the host? (The size of the time dim is often 3, so size of wk3_slc ~ 3n_xn_y*4. z=4 is for soil layers). Alternatively, I can modify lnd_iau_mod.F90 to read the values from file (during timestep_init) then interpolate and store intermediate values in "Land_IAU_state". BTW, is it correct to assume that Land_IAU_state is not affected by the memory issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

See tsga#1. Any module-level variables that need to persist when descoped need to be declared by the host unless those variables can effectively be shared across instances. Since the wk3* variables just contain the data read in by the host in the sfc_inc files (correct?), I'm not sure that they would need to be declared by the host. I.e., if different instances read in different sfc_inc files, then, yes, but otherwise, I think no.


! Read all increment files at iau init time (at beginning of cycle)
! allocate (wk3_stc(n_t, 1:im,jbeg:jend, 1:km))
call read_iau_forcing_fv3(Land_IAU_Control, errmsg, errflg) !, wk3_stc, wk3_slc
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't address this in tsga#1, but the code isn't checking for errors after returning from read_iau_forcing_fv3. When I try to run the new test with the data missing, instead of exiting with a nice error message, you get a seg fault because this code just continues and tries to access variables that were never allocated when the data files are missing.

return
end if
! Read Land IAU settings
call land_iau_mod_set_control(Land_IAU_Control, fn_nml, input_nml_file, &
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, it's a little awkward that the control for this functionality isn't read in until the physics init time. This means that every physics suite that uses NoahMP will at least have to go into the land_iau_mod_set_control routine to read the namelist. It would probably be cleaner to at least read in the do_land_iau variable in the host so that the physics can avoid doing anything with land IAU when it's not being used. I'm not aware of other physics schemes that force the host to read a namelist within the physics in order to know whether an option is used or not. I know that there is a GWD scheme that has a separate nml, but I think that there is also a namelist variable in the "main" physics namelist that controls whether the scheme namelist is even read. @dustinswales Do you have an opinion here?


! real (kind=kind_phys), dimension(max_soiltyp) :: maxsmc, bb, satpsi
! real, dimension(30) :: maxsmc, bb, satpsi
real(kind=kind_phys), parameter :: tfreez=273.16 !< con_t0c in physcons
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these physical constants being redefined here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants