Add capability to read soil increments on the jedi-based fv3 grid for synthetic soil temp and moisture DA experiments with noahmp by yuanxue2870 · Pull Request #871 · ufs-community/UFS_UTILS

yuanxue2870 · 2023-11-13T15:44:37Z

DESCRIPTION OF CHANGES:

add capability to read soil (i.e., both soil temp and soil moisture) increments from jedi-based fv3 grid directly
add capability to write out increments on the jedi-based fv3 grid from interpolating gsi-based soil increments on the Gaussian grid
revise calculate_landinc_mask to make it more stable and compatible with offline land DA workflow
add noahmp-related parameters for soil ice adjustment after ingesting soil temp increments
add jedi-based soil increments ingest and adjustment capability and separate from gsi-based ones
add a unit test (ftst_read_increments) to read jedi-based soil increments on the fv3 grid
revise the unit test (ftst_land_increments) to ingest soil temperature increments and adjust soil ice content (when needed)

ISSUE:

Fixes issues mentioned in #872

TESTS CONDUCTED:

Note: I only have access to Hera and Orion as of 11/13/2023.

Compile branch on all Tier 1 machines using Intel (Hera).
Compile branch on Hera using GNU.
Compile branch in 'Debug' mode on WCOSS2.
Run unit tests locally on any Tier 1 machine (Hera).
Run relevant consistency tests locally on all Tier 1 machine (Hera).
Ensure Doxygen builds with no warnings.

All my code edits should only affect global_cycle related unit tests and regression tests:

Regression tests: current tests1&3 are passed without issues, test2 will require an update in the baseline.
Unit tests: ftst_read_increments and ftst_land_increments are passed without issues.

…s directly

…xue2870/UFS_UTILS into feature/combine_w_offlineDA

GeorgeGayno-NOAA · 2023-11-13T16:24:14Z

@yuanxue2870 - did you open an issue for this?

@barlage - are you aware of this work?

yuanxue2870 · 2023-11-13T17:10:49Z

@yuanxue2870 - did you open an issue for this?

@barlage - are you aware of this work?

Hi George(@GeorgeGayno-NOAA)- Thanks for the inquiry! This is one of my assigned Redmine tasks, but I am not being assigned an issue nor opening an issue myself for this drafted PR.

I was discussing the best way to communicate my edits/revisions of the code with Mike over the past week. I also had a code-review meeting with Mike to walk through all my changes to the code. Mike suggested me only opening a draft PR at this stage and he wants to schedule a meeting with you and other key reviewers to briefly discuss this draft PR. Hope that clarifies things a little bit and we would like to discuss in detail when we have the meeting scheduled.

Thank you for your attention and help on this issue!

Best,
Yuan

…t to ingest soil temp increments and make adjustments

GeorgeGayno-NOAA · 2024-01-24T21:29:41Z

I am also getting a lot of Doxygen warnings in land_increment.f90.

Do you know how to turn on the Doxygen build? Modify the build script as follows:

diff --git a/build_all.sh b/build_all.sh
index 3d4bb080..df588160 100755
--- a/build_all.sh
+++ b/build_all.sh
@@ -49,7 +49,7 @@ if [[ ! -d "${DIR_ROOT}/ccpp-physics/physics" ]]; then
 fi

 # Collect BUILD Options
-CMAKE_FLAGS+=" -DCMAKE_BUILD_TYPE=${BUILD_TYPE:-Release}"
+CMAKE_FLAGS+=" -DENABLE_DOCS=ON -DCMAKE_BUILD_TYPE=${BUILD_TYPE:-Release}"

In most cases, the warnings occur because some of the routine arguments are not documented.

…nings

yuanxue2870 · 2024-01-25T17:36:44Z

I am also getting a lot of Doxygen warnings in land_increment.f90.

Do you know how to turn on the Doxygen build? Modify the build script as follows:
diff --git a/build_all.sh b/build_all.sh
index 3d4bb080..df588160 100755
--- a/build_all.sh
+++ b/build_all.sh
@@ -49,7 +49,7 @@ if [[ ! -d "${DIR_ROOT}/ccpp-physics/physics" ]]; then
 fi

 # Collect BUILD Options
-CMAKE_FLAGS+=" -DCMAKE_BUILD_TYPE=${BUILD_TYPE:-Release}"
+CMAKE_FLAGS+=" -DENABLE_DOCS=ON -DCMAKE_BUILD_TYPE=${BUILD_TYPE:-Release}"
In most cases, the warnings occur because some of the routine arguments are not documented.

@GeorgeGayno-NOAA: Thanks for your detailed instructions to help me re-build the application with Doxygen on. I have now added descriptions of routine arguments in land_increment.f90, read_write_data.f90, and set_soilveg_snippet.f90 to fully resolve all Doxygen-prompted warnings associated with this PR.

GeorgeGayno-NOAA · 2024-01-25T18:43:23Z

I am getting an odd warning when I compile with Intel:

/scratch1/NCEPDEV/da/George.Gayno/ufs_utils.git/UFS_UTILS.yuanxue/sorc/global_cycle.fd/land_increments.f90(1): warning #5462: Global name too long, shortened from: land_increments_mp_apply_land_da_adjustments_soil_temp_$blk.mpi_constants_mp_mpi_statuses_ignore_ to: lnd_increments_mp_apply_land_da_adjustments_soil_temp_$blk.mpi_constants_mp_mpi_statuses_ignore_
!> @file

I don't think it a problem. And according to the following link, the warning can be eliminated by shortening the subroutine names in that module:
https://community.intel.com/t5/Intel-Fortran-Compiler/Many-quot-Global-name-too-long-quot-warnings/td-p/1505843

I don't want you to change your code for this warning, but I worry that NCO will reject a future UFS_UTILS code delivery because of it. The warning can be suppressed by adding a compiler option. Can you update the cmake file as follows:

--- a/sorc/global_cycle.fd/CMakeLists.txt
+++ b/sorc/global_cycle.fd/CMakeLists.txt
@@ -14,7 +14,7 @@ set(lib_src
 set(exe_src cycle.f90)

 if(CMAKE_Fortran_COMPILER_ID MATCHES "^(Intel)$")
-  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -r8")
+  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -diag-disable:5462 -r8")
 elseif(CMAKE_Fortran_COMPILER_ID MATCHES "^(GNU)$")
   set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -fdefault-real-8")
   if(CMAKE_Fortran_COMPILER_VERSION VERSION_GREATER_EQUAL 10)

GeorgeGayno-NOAA · 2024-01-25T18:43:51Z

Your branch is out-of-date. Please merge the latest updates from 'develop' to your branch.

yuanxue2870 · 2024-01-25T18:58:19Z

I am getting an odd warning when I compile with Intel:
/scratch1/NCEPDEV/da/George.Gayno/ufs_utils.git/UFS_UTILS.yuanxue/sorc/global_cycle.fd/land_increments.f90(1): warning #5462: Global name too long, shortened from: land_increments_mp_apply_land_da_adjustments_soil_temp_$blk.mpi_constants_mp_mpi_statuses_ignore_ to: lnd_increments_mp_apply_land_da_adjustments_soil_temp_$blk.mpi_constants_mp_mpi_statuses_ignore_
!> @file
I don't think it a problem. And according to the following link, the warning can be eliminated by shortening the subroutine names in that module: https://community.intel.com/t5/Intel-Fortran-Compiler/Many-quot-Global-name-too-long-quot-warnings/td-p/1505843

I don't want you to change your code for this warning, but I worry that NCO will reject a future UFS_UTILS code delivery because of it. The warning can be suppressed by adding a compiler option. Can you update the cmake file as follows:
--- a/sorc/global_cycle.fd/CMakeLists.txt
+++ b/sorc/global_cycle.fd/CMakeLists.txt
@@ -14,7 +14,7 @@ set(lib_src
 set(exe_src cycle.f90)

 if(CMAKE_Fortran_COMPILER_ID MATCHES "^(Intel)$")
-  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -r8")
+  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -diag-disable:5462 -r8")
 elseif(CMAKE_Fortran_COMPILER_ID MATCHES "^(GNU)$")
   set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -fdefault-real-8")
   if(CMAKE_Fortran_COMPILER_VERSION VERSION_GREATER_EQUAL 10)

Done. Thanks. Once the scientific validity of this PR is confirmed by all reviewers, I will shorten the relevant routine name to fully resolve this warning.

… routine name

yuanxue2870 · 2024-01-25T20:08:50Z

Your branch is out-of-date. Please merge the latest updates from 'develop' to your branch.

When merging, my current branch has a conflict with ab9b0b9. I need some time to figure out how to merge the latest 'develop" to my branch. Thanks!

ClaraDraper-NOAA

Thanks @yuanxue2870 this is looking good. See my comments and questions (most of the questions are just for my knowlege). The big changes are: I think we're only reading in the JEDI increment from tile 1 on all processors; and I'm not sure what the value is of changing the order of operations for the SMC / STC updates. We also need to use the same method for the GSI and JEDI, since the whole point is to test what I'm doing for GSI.

ClaraDraper-NOAA · 2024-01-25T22:33:48Z

-!!  -DO_SNO_INC     Do land increments to snow states.
+!!  -DO_SOI_INC_GSI   Do land increments (on the Guassian grid) to soil states.
+!!  -DO_SOI_INC_JEDI  Do land increments (on the cubed sphere tiles) to soil.
+!!  -DO_SNO_INC       Do land increments (on the cubed sphere tiles) to snow states.


For consistency with the new soil increment flags, please change this to DO_SNO_INC_JEDI.

ClaraDraper-NOAA · 2024-01-25T22:47:12Z

+       DO_SOI_INC_GSI = .TRUE.
       PRINT*
       PRINT*," APPLYING SOIL INCREMENTS FROM THE GSI"
-       ALLOCATE(STC_BCK(LENSFC, LSOIL), SMC_BCK(LENSFC, LSOIL), SLC_BCK(LENSFC,LSOIL))


I find the logic here a bit contrived. I seem to have coded this to have an option where DO_LNDINC is on, but then we skip it if the file name is not specified in the namelist. I'm not sure why I did this, but it doesn't make a lot of sense to me now. I'd also like to have a more consistent methods for both options (JEDI, GSI). I suggest changing the namelist to have two input options: DO_SOI_INC_GSI and DO_SOI_INC_JEDI, add then just assume standard file names for each (the GSI one is called, lnd_incr). Add a check to make sure both aren't specified as true, and have the code crash if the file is missing.

Per discussion, the DO_LNDINC will be kept as it is. Separate DO_SOI_INC_GSI and DO_SOI_INC_JEDI. Add options in the namelist file.

ClaraDraper-NOAA · 2024-01-25T22:48:46Z

+   ELSE
+       INQUIRE(FILE='xainc.001', EXIST=file_exists)
+       IF (file_exists) then
+         IERR=NF90_OPEN('xainc.001',NF90_NOWRITE,NCID)


This looks like it's only opening the JEDI increment from tile number 1 (right?). This will need to be generalized to open the appropriate file (with the number coming form the task numbers - copy how it's done for opening the background).

ClaraDraper-NOAA · 2024-01-25T22:49:36Z

+       INQUIRE(FILE='xainc.001', EXIST=file_exists)
+       IF (file_exists) then
+         IERR=NF90_OPEN('xainc.001',NF90_NOWRITE,NCID)
+         IERR=NF90_INQ_VARID(NCID,"stc_inc",ID_VAR)


Why would stc_inc not be on the file, and why do we specifically check for it?

ClaraDraper-NOAA · 2024-01-25T22:52:04Z

+   ALLOCATE(STC_BCK(LENSFC, LSOIL), SMC_BCK(LENSFC, LSOIL), SLC_BCK(LENSFC,LSOIL))
+   ALLOCATE(LANDINC_MASK_FG(LENSFC))
   ! FOR NOW, CODE SO CAN DO BOTH, BUT MIGHT NEED TO THINK ABOUT THIS SOME MORE.
+   ! NOTE: BOTH snow and soil related increment file are named as "xainc.00N"


This answers my earlier question! If we're going to assume file names here, it would be best to use two different names. The increment file name is specified in the JEDI yaml, so easy to change at that end.

ClaraDraper-NOAA · 2024-01-25T23:13:36Z

+                                               STC_BCK, STCFCS, SMCFCS, SLCFCS,&
+                                               STC_UPDATED)
+
+          CALL ADD_JEDI_INCREMENT_SOIL_MOISTURE(SLCINC, STCFCS, SMCFCS, SLCFCS,&


I see the three-step process now. So it's not re-calling JEDI. Let's talk about why you did this, but my impulse is that it makes it more complicated, without really adding any advantages.

Will maintain the original logic.

ClaraDraper-NOAA · 2024-01-25T23:24:16Z

+   !! @param[in] slc_inc_tmp Liquid soil moisture increments on the cubed-sphere tiles
   !! @author George Gayno NOAA/EMC
-
+   !! @author Yuan Xue: gsi-based soil increments are interpolated onto the


Unless you're using it, I don't think we need this (we can diff the anal and back to get the increment). I think my comment about doing this was out-dated. I suppose we may as well leave it in, in case it's useful in the future?

Will leave it for now per discussion.

ClaraDraper-NOAA · 2024-01-25T23:25:41Z


 write(rankch, '(i3.3)') (myrank+1)

- fnbgso = "./fnbgso." // rankch


This is an example of how the file names are specified with the rank on the end. In some cases the rank is the file, in others it's the ensemble member.

Ignore per discussion.

ClaraDraper-NOAA · 2024-01-25T23:26:31Z


+ else
+
+    fnbgso = "./xainc." // rankch


Let's give this a different file name. Say "gauss_interp".

ClaraDraper-NOAA · 2024-01-25T23:29:28Z

 !! @param[out] satpsi Saturated matric potential for each soil type
 !! @param[out] iret Return integer
-subroutine set_soilveg(isot,ivet, maxsmc, bb, satpsi, iret) 
+subroutine set_soilveg_noah(isot,ivet, maxsmc, bb, satpsi, iret) 


This is great, thanks. I'll ask you in person about what has changed (specifically, what errors might be in my experiments so far).

yuanxue2870 · 2024-01-26T19:27:30Z

Thanks both of your help, @GeorgeGayno-NOAA and @ClaraDraper-NOAA for reviewing this! I had a meeting with @ClaraDraper-NOAA on 01/26/2024 afternoon to go over all of her comments one-by-one and clarify/confirm the unclear part. I will now make this PR as a draft to address all of the comments. Once I am done, I will convert it back to Open and let both of you know in order to re-review it. Thanks!!

yuanxue2870 · 2024-02-10T14:45:37Z

This PR is associated with the branch of feature/combine_w_offlineDA as the goal was to combine UFS_UTILS and offline land DA workflow for a series of synthetic twin DA experiments to figure out the best option for soil ice adjustment during frozen to unfrozen transitions. This branch includes some hacks to make UFS_UTILS/global_cycle to be compatible with current version of ufs-land-driver, which should not be incorporated into current UFS_UTILS/global_cycle as it may degrade its functionality. Yuan Xue discussed this issue with Clara Draper first on 01/26/2024 and we thought it might be best to leave the feature/combine_w_offlineDA as a separate repo instead. Yuan Xue further discussed this issue with Mike Barlage and Geroge Gayno on 02/09/2024 and we all agreed with the action.

Hence, Yuan Xue closes PR#871, and opens a new PR for the branch feature/soil_ice_adjust to address the soil ice adjustment and fv3-grid soil increment reading issue#872 (#872). All comments from all reviewers are incorporated in the new PR. Please feel free to contact yuan.xue@noaa.gov if there are any further comments/concerns regarding this. Thank you!

yuanxue2870 and others added 9 commits November 6, 2023 19:41

separate gsi based soil increment from jedi based soil increment

f7a2cff

add capability to read soil related increments from cubed sphere tile…

04bbba0

…s directly

Merge branch 'ufs-community:develop' into feature/combine_w_offlineDA

8d407eb

output gsi-based increments onto cubed sphere tiles

415b71c

Merge branch 'feature/combine_w_offlineDA' of https://github.com/yuan…

6026321

…xue2870/UFS_UTILS into feature/combine_w_offlineDA

revise the mask calculation scheme to make it more stable

3e0f572

relax the noah-mp lsm checkup to combine with ufs-land-driver

8e30a20

add noah-mp soilveg parameters and separate from noah ones

6f7d5d6

add jedi related soil increments ingest and adjustment capability

9ff83e0

add a unit test to read jedi-based soil incr. and revise the unit tes…

1c9593a

…t to ingest soil temp increments and make adjustments

yuanxue2870 mentioned this pull request Nov 13, 2023

Add capability to read soil increments on the cubed-sphere tiles and enhance soil ice adjustment due to soil temperature increments #872

Closed

GeorgeGayno-NOAA self-requested a review November 17, 2023 20:38

yuanxue2870 marked this pull request as ready for review January 18, 2024 14:51

yuanxue2870 added 2 commits January 25, 2024 17:19

add detailed descriptions to routine arguments to resolve Doxygen war…

15b3120

…nings

continue resolving Doxygen warnings

65c62d8

replace cmake fortran flag to suppress warnings related to a too-long…

0998637

… routine name

ClaraDraper-NOAA suggested changes Jan 25, 2024

View reviewed changes

yuanxue2870 marked this pull request as draft January 26, 2024 19:31

Delete tests/global_cycle/data directory

e921906

yuanxue2870 closed this Feb 10, 2024

yuanxue2870 mentioned this pull request Feb 10, 2024

Add capability to read soil increments on the cubed-sphere tiles and enhance soil ice adjustment due to soil temperature increments #894

Merged

5 tasks


		write(rankch, '(i3.3)') (myrank+1)

		fnbgso = "./fnbgso." // rankch

Conversation

yuanxue2870 commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DESCRIPTION OF CHANGES:

ISSUE:

TESTS CONDUCTED:

Uh oh!

GeorgeGayno-NOAA commented Nov 13, 2023

Uh oh!

yuanxue2870 commented Nov 13, 2023

Uh oh!

GeorgeGayno-NOAA commented Jan 24, 2024

Uh oh!

yuanxue2870 commented Jan 25, 2024

Uh oh!

GeorgeGayno-NOAA commented Jan 25, 2024

Uh oh!

GeorgeGayno-NOAA commented Jan 25, 2024

Uh oh!

yuanxue2870 commented Jan 25, 2024

Uh oh!

yuanxue2870 commented Jan 25, 2024

Uh oh!

ClaraDraper-NOAA left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuanxue2870 commented Jan 26, 2024

Uh oh!

yuanxue2870 commented Feb 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yuanxue2870 commented Nov 13, 2023 •

edited

Loading