Feature/snow incr noah #568

ClaraDraper-NOAA · 2021-08-16T22:59:34Z

This addresses issue #561
Addition of routines to add snow depth increments from JEDI output increment file to the sfc restarts. Includes screening to apply updates over land only and adjusting SWE to match the snow depth analysis. Format follows previous approach for soil temperature increments.
Associated changes:

made arguments to read_data optional so could use same routine to
read JEDI increment field
changed confusing variable names for SNO (-> SWE) and SWE -> (SND)
Added new routines to 1) add snow depth increments, and ii) adjust SWE increments accordingly.
Updated the land mask already used by the soil temperature DA to include land-ice

Note: no action is taken if grid cell is a land-ice point. The current snow DA limits snow depth and SWE to be above a minimum value over land-ice, which is also done at every Noah call, so I skipped it.

Also: this won't work with a fractional grid cells, but I don't know enough about how that's handled in the model to address it. I suggest we address this next.

to the sfc restarts. Includes screening to apply updates over land only and adjusting SWE to match the snow depth analysis. Also: * made arguments to read_data optional so could use same routine to read JEDI increment field * changed confusing variable names for SNO (-> SWE) and SWE -> (SND)

ClaraDraper-NOAA · 2021-08-16T23:00:47Z

I don't seem to be able to add reviewers. @barlage and @jiaruidong2017 should be included.

GeorgeGayno-NOAA · 2021-08-17T13:49:39Z

@ClaraDraper-NOAA Do you have a canned case to test this option?

ClaraDraper-NOAA · 2021-08-17T15:35:35Z

@GeorgeGayno-NOAA apologies - I knew there was something I'd forgotten to include, but couldn't figure out what it was. The test case is on hera (see readme for instructions)
/scratch2/BMC/gsienkf/Clara.Draper/DA_test_cases/snow/add_incr/

GeorgeGayno-NOAA · 2021-08-17T18:27:18Z

I see the first regression test is failing:

 ADJUST TREF FROM GSI INCREMENT
mon1,mon2,rjday,wei1,wei2=   7   8  211.000    0.532    0.468
 mlat_sst, mlon_sst :         2160        4320
  sstclm : sstclm
  sstclm : sstclm

 MAXIMUM SEARCH IS           20  GAUSSIAN POINTS.

forrtl: severe (174): SIGSEGV, segmentation fault occurred
Image              PC                Routine            Line        Source
global_cycle       000000000091779D  Unknown               Unknown  Unknown
libpthread-2.17.s  00002AC7D9E34630  Unknown               Unknown  Unknown
global_cycle       0000000000411674  adjust_nsst_             1009  cycle.f90
global_cycle       000000000040ACD5  sfcdrv_                   569  cycle.f90
global_cycle       0000000000407A8B  MAIN__                    172  cycle.f90
global_cycle       000000000040705E  Unknown               Unknown  Unknown
libc-2.17.so       00002AC7DACFF555  __libc_start_main     Unknown  Unknown
global_cycle       0000000000406F69  Unknown               Unknown  Unknown

ClaraDraper-NOAA · 2021-08-17T19:13:36Z

Let me have a look, and fix it.

GeorgeGayno-NOAA · 2021-08-17T19:44:15Z

@GeorgeGayno-NOAA apologies - I knew there was something I'd forgotten to include, but couldn't figure out what it was. The test case is on hera (see readme for instructions)
/scratch2/BMC/gsienkf/Clara.Draper/DA_test_cases/snow/add_incr/

Great. I was able to get it to run. This should be made into a consistency/regression test?

GeorgeGayno-NOAA · 2021-08-17T19:52:25Z

I see you added a new variable - LND_SNO_FILE - to the fort.37 namelist. Don't forget to update ./ush/global_cycle.sh accordingly:

https://github.com/ClaraDraper-NOAA/UFS_UTILS/blob/feature/snow_incr_Noah/ush/global_cycle.sh#L388

GeorgeGayno-NOAA · 2021-08-17T20:01:26Z

This addresses issue #561
Addition of routines to add snow depth increments from JEDI output increment file to the sfc restarts. Includes screening to apply updates over land only and adjusting SWE to match the snow depth analysis. Format follows previous approach for soil temperature increments.
Associated changes:

made arguments to read_data optional so could use same routine to
read JEDI increment field

changed confusing variable names for SNO (-> SWE) and SWE -> (SND)

Added new routines to 1) add snow depth increments, and ii) adjust SWE increments accordingly.

Updated the land mask already used by the soil temperature DA to include land-ice

Note: no action is taken if grid cell is a land-ice point. The current snow DA limits snow depth and SWE to be above a minimum value over land-ice, which is also done at every Noah call, so I skipped it.

Also: this won't work with a fractional grid cells, but I don't know enough about how that's handled in the model to address it. I suggest we address this next.

There is an issue open to convert global_cycle for fractional grids: #549

ClaraDraper-NOAA · 2021-08-17T20:14:25Z

@GeorgeGayno-NOAA apologies - I knew there was something I'd forgotten to include, but couldn't figure out what it was. The test case is on hera (see readme for instructions)
/scratch2/BMC/gsienkf/Clara.Draper/DA_test_cases/snow/add_incr/

Great. I was able to get it to run. This should be made into a consistency/regression test?

I think this should be a regression test. Or I could create a single RT with both the soil and snow updates? (this doesn't currently make sense scientifically, but would exercise both bits of code in a single test).

ClaraDraper-NOAA · 2021-08-17T20:14:39Z

I see you added a new variable - LND_SNO_FILE - to the fort.37 namelist. Don't forget to update ./ush/global_cycle.sh accordingly:

https://github.com/ClaraDraper-NOAA/UFS_UTILS/blob/feature/snow_incr_Noah/ush/global_cycle.sh#L388

Will do.

GeorgeGayno-NOAA · 2021-08-17T20:18:52Z

@GeorgeGayno-NOAA apologies - I knew there was something I'd forgotten to include, but couldn't figure out what it was. The test case is on hera (see readme for instructions)
/scratch2/BMC/gsienkf/Clara.Draper/DA_test_cases/snow/add_incr/

Great. I was able to get it to run. This should be made into a consistency/regression test?

I think this should be a regression test. Or I could create a single RT with both the soil and snow updates? (this doesn't currently make sense scientifically, but would exercise both bits of code in a single test).

Let's try separate tests - global_cycle runs quickly. I can help with machines you don't have access to.

GeorgeGayno-NOAA · 2021-08-17T21:18:40Z

Although not specific to this PR, I see several doxygen warnings for ./lsm_routines.fd/noah.fd/sflx_snippet.f90. I am able to fix them by removing these comment lines:

--- a/sorc/lsm_routines.fd/noah.fd/sflx_snippet.f90
+++ b/sorc/lsm_routines.fd/noah.fd/sflx_snippet.f90
@@ -25,13 +25,7 @@
 !! @param[in] bexp B exponent
 !! @param[in] psis Saturated matric potential
 !! @param[out]  liqwat Output liquid soil moisture
-      subroutine frh2o                                                  &
-!  ---  inputs:
-     &     ( tkelv, smc, sh2o, smcmax, bexp, psis,                      &
-!  ---  outputs:
-     &       liqwat                                                     &
-     &     )
-
+      subroutine frh2o ( tkelv, smc, sh2o, smcmax, bexp, psis,liqwat)
 ! ===================================================================== !

…to scripts.

ClaraDraper-NOAA · 2021-08-18T19:46:55Z

This addresses issue #561
Addition of routines to add snow depth increments from JEDI output increment file to the sfc restarts. Includes screening to apply updates over land only and adjusting SWE to match the snow depth analysis. Format follows previous approach for soil temperature increments.
Associated changes:

made arguments to read_data optional so could use same routine to
read JEDI increment field

changed confusing variable names for SNO (-> SWE) and SWE -> (SND)

Added new routines to 1) add snow depth increments, and ii) adjust SWE increments accordingly.

Updated the land mask already used by the soil temperature DA to include land-ice

Note: no action is taken if grid cell is a land-ice point. The current snow DA limits snow depth and SWE to be above a minimum value over land-ice, which is also done at every Noah call, so I skipped it.
Also: this won't work with a fractional grid cells, but I don't know enough about how that's handled in the model to address it. I suggest we address this next.

There is an issue open to convert global_cycle for fractional grids: #549

OK. I suspect that the way I've created the mask in calculate_landinc_mask will need to be revised.

ClaraDraper-NOAA · 2021-08-18T19:57:46Z

@GeorgeGayno-NOAA I fixed the bug (that took a bit of searching!), and updated the global_cycle_driver scripts with the new namelist variable. The global_cycle regression tests now pass. I'm creating an rg test on hera now. I have an orion account now, so I can do that one too.

/scratch2/BMC/gsienkf/Clara.Draper/tmp/reg-tests/global-cycle/global_cycle

ClaraDraper-NOAA · 2021-08-19T21:27:50Z

@GeorgeGayno-NOAA I've done the regression_tests for orion and hera. The files you'll need are on here here:

/scratch2/BMC/gsienkf/Clara.Draper/tmp/reg-tests/global-cycle/global_cycle

I just ran the tests on orion using the baseline for the new test from hera, and they passed.

Note the test makes no science sense, but it should be enough to exercise the code. I used increment files generated during winter (so that the increment would be non-zero) and applied them to the pre-existing background files in boreal summer. If you want something more scientifically sound, we can replace the background for that test with ones for the same day as the increments.

I think everything else is ready to go?

GeorgeGayno-NOAA · 2021-08-20T17:55:11Z

@GeorgeGayno-NOAA I've done the regression_tests for orion and hera. The files you'll need are on here here:

/scratch2/BMC/gsienkf/Clara.Draper/tmp/reg-tests/global-cycle/global_cycle

I just ran the tests on orion using the baseline for the new test from hera, and they passed.

Note the test makes no science sense, but it should be enough to exercise the code. I used increment files generated during winter (so that the increment would be non-zero) and applied them to the pre-existing background files in boreal summer. If you want something more scientifically sound, we can replace the background for that test with ones for the same day as the increments.

I think everything else is ready to go?

I was able to run your tests on Hera. If you prefer to use winter background files, I have no problem with keeping multiple directories of input data. We already do that for other regression tests.

GeorgeGayno-NOAA · 2021-08-20T21:19:51Z

@ClaraDraper-NOAA I will check in some minor script updates to your branch.

Fixes ufs-community#561

GeorgeGayno-NOAA · 2021-08-24T13:47:10Z

@ClaraDraper-NOAA I just merged the latest updates from develop to your branch. I see the consistency test driver scripts need to be updated for Orion, Jet, and WCOSS. I can do WCOSS.

Fixes ufs-community#561

Fixes ufs-community#561.

ClaraDraper-NOAA · 2021-08-24T16:24:15Z

Thanks @GeorgeGayno-NOAA. I'll do orion and jet later today.

GeorgeGayno-NOAA · 2021-08-24T17:36:30Z

Thanks @GeorgeGayno-NOAA. I'll do orion and jet later today.

I have Jet done. Will check in next. Orion is down today.

Fixes ufs-community#561

Fixes ufs-community#561.

sorc/global_cycle.fd/cycle.f90

GeorgeGayno-NOAA · 2021-08-24T19:45:38Z

@ClaraDraper-NOAA Other than a few minor prolog updates, I have no further comments. I did not look closely at the science behind your updates. I will let @barlage and @jiaruidong2017 check that.

ClaraDraper-NOAA · 2021-08-24T20:27:54Z

Thanks @GeorgeGayno-NOAA. I just pushed the changes to the prologue.

jiaruidong2017

It's good to me for this PR by adding the snow increments with a proposed basic method. It may require some detailed considerations after the snow increments are available in the future analysis.

GeorgeGayno-NOAA · 2021-09-02T14:25:18Z

Thanks @jiaruidong2017

@ClaraDraper-NOAA I will merge the latest updates from 'develop' to your branch. Then merge.

barlage

A few minor comments.

barlage · 2021-09-02T14:29:34Z

sorc/global_cycle.fd/cycle.f90

-! to the changes made. Need to store bacground, apply the increments, then make 
-! secondart adjustments. When updating more than one state, be careful about the 
-! orfer if increments and secondary adjustments. 
+        LANDINC_MASK_FG = LANDINC_MASK



I assume what is happening here is that soil updates are not done where there is snow. Is that stated anywhere? If not, it would be good to have in a comment.

@barlage I moved the comments below from near the do loop, to the comment block at the top of the add_increment_soil functions.

!! Does not make a temperature update if snow differ
!! between fg and anal (allow correction of snow to
!! address temperature error first), or if snow is present
!! (will eventually updating of snow temperature in this case)

The code also counts, and prints out the number of locations under each case.

barlage · 2021-09-02T14:35:29Z

sorc/global_cycle.fd/cycle.f90

+        STC_BCK = STCFCS
+        SMC_BCK = SMCFCS ! not used yet.
+        SLC_BCK = SLCFCS ! not used yet.
+


Any reason to not generalize this for all three soil states now?

Not necessary, just wondering if it saves any work in the future.

Doing the soil moisture will require knowledge of the soil properties (for re-gridding increments), so I'll need to add them to the increment file output by the GSI. There's a bit of work to do there, so I'm not planning to do it yet (and may even be able to old off until we're using JEDI).

barlage · 2021-09-02T14:46:52Z

sorc/global_cycle.fd/land_increments.f90

    write(*,'(a,i8)') ' (not updated) soil grid cells, no soil nearby on gsi grid = ',nnosoilnear
    write(*,'(a,i8)') ' (not updated) soil grid cells, change in presence of snow = ', nsnowchange
    write(*,'(a,i8)') ' (not updated yet) snow grid cells = ', nsnowupd
-    write(*,'(a,i8)') ' grid cells, without soil of snow = ', nother 
+    write(*,'(a,i8)') ' grid cells, without soil of snow = ', nother


"of" to "or"?

should be "or". fixed.

barlage · 2021-09-02T14:48:32Z

sorc/global_cycle.fd/land_increments.f90

-!> Calculate soil mask for land on model grid. 
-!! Output is 1  - soil, 2 - snow-covered, 0 - land ice or not land.
-!! @param[in] lensfc  Total numberof points for the cubed-sphere tile.
+ !> Add soil snow depth increment to model snow depth state,


remove "soil"? Also, "genative", "globall" and "screend" below.

barlage · 2021-09-02T14:52:26Z

sorc/global_cycle.fd/land_increments.f90

    do i=1,lensfc
-        if (swe(i) .GT. 0.001) then
-        mask(i) = 2
+        if (smc(i) .LT. 1.0) then


is this to screen water points? what happens for fractional grid? I don't know myself.

A soil moisture of 1.0 indicates permanent land ice. I think @ClaraDraper-NOAA does not want to apply the updates at those points.

@barlage I'm not sure what happens for the fractional grid either. I flagged to @GeorgeGayno-NOAA that I didn't think my code would work for fractional grid cells, and I think he's planning to this update this when all of UFS_UTILS is updated?

I have an issue open to convert this program to process fractional grids - #549.

barlage · 2021-09-02T14:59:07Z

sorc/lsm_routines.fd/noah.fd/bulk_snow_module.f90

+       real :: dens_mean
+
+        ! density = swe/snd
+        density = swe/snd


any chance of a divide by zero here?

I think they called the "calc_density" subroutine where the snow present.

We don't use any of these values is there's no snow, but "no snow" is based on SWE, so it could be possible if we somehow wound up with 0.0 snow depth, and non-zero SWE. I replaced that code with a loop, checking on the SND value, just to make sure.

I will hold off merging until @ClaraDraper-NOAA addresses the latest comments.

@barlage @GeorgeGayno-NOAA Just pushed responses to Mike's comments. I'll keep an eye on the automatic checks ( there's a small code change to the snow density calculation, but it passed the snow increment reg_test)

GeorgeGayno-NOAA · 2021-09-02T17:47:40Z

I will hold off merging until @ClaraDraper-NOAA addresses the latest comments.

GeorgeGayno-NOAA · 2021-09-02T21:59:17Z

@ClaraDraper-NOAA Your latest updates to not change the regression tests. I will merge tomorrow morning.

ClaraDraper-NOAA added 11 commits August 13, 2021 21:00

Added comment.

bd51709

Doxygen bugfix.

2aad1e9

Another doxygen bugfix.

be5d862

More doxygen fixes.

f583e15

Trying again.

0453a41

doxygen.

4c3378b

doxygen

0aa7e2d

doxygen

851a68b

reverting to original version.

ddb28ee

Doxygen

4b122b5

GeorgeGayno-NOAA requested review from barlage, jiaruidong2017 and GeorgeGayno-NOAA August 17, 2021 12:08

Fixed bug in read_data for NSST, and added LND_SNO_FILE nml variable …

4159d87

…to scripts.

Added rt test on hera. Additional files are here:

9f6228b

/scratch2/BMC/gsienkf/Clara.Draper/tmp/reg-tests/global-cycle/global_cycle

Moved nml up to declarations.

1c1134b

GeorgeGayno-NOAA and others added 2 commits August 20, 2021 21:22

Minor script updates.

a881dad

Fixes ufs-community#561

Merge branch 'develop' into feature/snow_incr_Noah

ee425f9

GeorgeGayno-NOAA added 2 commits August 24, 2021 14:22

Update consistency test driver script for WCOSS-Dell.

b29aa2a

Fixes ufs-community#561

Update consistency test driver script for Cray.

956b259

Fixes ufs-community#561.

GeorgeGayno-NOAA added 2 commits August 24, 2021 17:47

Update consistency test driver script for Jet.

8e8d495

Fixes ufs-community#561

Update regression test driver script for Orion.

589ba00

Fixes ufs-community#561.

GeorgeGayno-NOAA reviewed Aug 24, 2021

View reviewed changes

sorc/global_cycle.fd/cycle.f90 Show resolved Hide resolved

GeorgeGayno-NOAA reviewed Aug 24, 2021

View reviewed changes

sorc/global_cycle.fd/cycle.f90 Show resolved Hide resolved

prologue updates.

bf3efd8

Doxygen bug?

0197f82

jiaruidong2017 approved these changes Sep 2, 2021

View reviewed changes

Merge branch 'develop' into feature/snow_incr_Noah

85a441e

barlage approved these changes Sep 2, 2021

View reviewed changes

updates from Mike Barlage's code review.

a337ddb

GeorgeGayno-NOAA self-requested a review September 2, 2021 22:00

GeorgeGayno-NOAA approved these changes Sep 2, 2021

View reviewed changes

GeorgeGayno-NOAA merged commit dea8dd9 into ufs-community:develop Sep 3, 2021

ClaraDraper-NOAA deleted the feature/snow_incr_Noah branch August 7, 2024 14:18

Feature/snow incr noah #568

Feature/snow incr noah #568

Conversation

ClaraDraper-NOAA commented Aug 16, 2021

ClaraDraper-NOAA commented Aug 16, 2021

GeorgeGayno-NOAA commented Aug 17, 2021

ClaraDraper-NOAA commented Aug 17, 2021

GeorgeGayno-NOAA commented Aug 17, 2021

ClaraDraper-NOAA commented Aug 17, 2021

GeorgeGayno-NOAA commented Aug 17, 2021

GeorgeGayno-NOAA commented Aug 17, 2021

GeorgeGayno-NOAA commented Aug 17, 2021

ClaraDraper-NOAA commented Aug 17, 2021

ClaraDraper-NOAA commented Aug 17, 2021

GeorgeGayno-NOAA commented Aug 17, 2021

GeorgeGayno-NOAA commented Aug 17, 2021

ClaraDraper-NOAA commented Aug 18, 2021

ClaraDraper-NOAA commented Aug 18, 2021

ClaraDraper-NOAA commented Aug 19, 2021

GeorgeGayno-NOAA commented Aug 20, 2021

GeorgeGayno-NOAA commented Aug 20, 2021

GeorgeGayno-NOAA commented Aug 24, 2021

ClaraDraper-NOAA commented Aug 24, 2021

GeorgeGayno-NOAA commented Aug 24, 2021

GeorgeGayno-NOAA commented Aug 24, 2021

ClaraDraper-NOAA commented Aug 24, 2021

jiaruidong2017 left a comment

Choose a reason for hiding this comment

GeorgeGayno-NOAA commented Sep 2, 2021

barlage 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

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

GeorgeGayno-NOAA commented Sep 2, 2021

GeorgeGayno-NOAA commented Sep 2, 2021 • edited Loading

GeorgeGayno-NOAA commented Sep 2, 2021 •

edited

Loading