Snow burial fix #1112
Snow burial fix #1112
Conversation
billsacks
added
PR status: ready
|
|
billsacks
removed
the
next
|
Also, maybe slightly unrelated to this PR, but I'm wondering if you (@danicalombardozzi ) or others (@dlawrenncar @swensosc , @wwieder , etc.) know whether this difference is intentional or unintentional: For Satellite Phenology, I see: CTSM/src/biogeochem/SatellitePhenologyMod.F90 Lines 419 to 420 in 32964a8 whereas for BGC, there is no accounting for frac_sno in the similar lines: CTSM/src/biogeochem/CNVegStructUpdateMod.F90 Lines 295 to 296 in 32964a8
|
|
That looks like a bug to me. frac_sno should be used in both places. |
|
I'm comfortable folding in this other bug fix (if others agree that is indeed a bug) with this branch, if others are comfortable with that. Any feelings on whether there should be a simulation looking at the scientific impacts of (a) the snow burial fix for BGC and/or (b) the apparent bug with frac_sno in elai and esai – either together or separately? Or do people feel that a dedicated simulation for this is unnecessary and we'll just look at the impact of these changes together with some other answer changes? |
|
@danicalombardozzi how big was the effect on of this bugfix on your SP runs? If it's large, it may be a good idea to test the effects in a historical simulation before just folding it into the model? If the SP effect is small, maybe the BGC tests aren't necessary? This would matter for the arctic, however, making me think a BGC test is needed? |
|
I guess I think that we might be able to fold in the assessment of this fix
with our new test runs with the CLM5PPE tag. It's a clear bug so there
isn't really any question about whether or not we should fix it, so the
only option if simulation looks bad would be to retune. I would anticipate
that the impact will not be big because the impact is mainly restricted to
a short period during the melt season
…On Thu, Aug 20, 2020 at 12:48 PM will wieder ***@***.***> wrote:
@danicalombardozzi <https://github.com/danicalombardozzi> how big was the
effect on of this bugfix on your SP runs? If it's large, it may be a good
idea to test the effects in a historical simulation before just folding it
into the model? If the SP effect is small, maybe the BGC tests aren't
necessary?
This would matter for the arctic, however, making me think a BGC test is
needed?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFABYVBG5JQ6FORZGL6QASLSBVVYFANCNFSM4QFM6VOA>
.
|
|
I will plan to add the bug in the BGC code into this tag. Folding the fixes into the CLM5PPE tag seems reasonable. I can also run simulations with and without the fix. I will add a comment about the 20% lodging rate, although it was an assumption I made for my 2018 GRL cover crop paper and is not based on data (I couldn't find available data). |
…ded photosynthesis
…lculation, fix for bug ESCOMP#516
| !grass and crop snow burial changes with PFT height | ||
| !accounts for a 20% bending factor, as used in Lombardozzi et al. (2018) GRL 45(18), 9889-9897 |
There was a problem hiding this comment.
- I think this comment should be a few lines up, where you have changed fb (line 413)
| !grass and crop snow burial changes with PFT height | ||
| !accounts for a 20% bending factor, as used in Lombardozzi et al. (2018) GRL 45(18), 9889-9897 |
There was a problem hiding this comment.
-
As in SatellitePhenologyMod, I think this comment belongs a few lines up, with the fb setting
-
More importantly, it looks like you haven't fixed fb here as you did for the SatellitePhenology code. (Maybe you just haven't done that step yet.)
|
I ran two 5-year simulations using an I2000 BGC compset with no spin up. The simulations tested 1) the quadratic bug fix #756 and 2) the snow burial bug fix for BGC #516 (which also includes the quadratic bug fix). Both simulations change answers. The quadratic bug fix answer changes are small in magnitude, whereas the snow burial bug fixes cause larger changes. Analysis with plots illustrating the changes are here: |
This contains the changes to fix ESMCI/cime#3769, equivalent to ESMCI/cime#3770. Resolves ESCOMP#1203
|
@danicalombardozzi @olyson Unfortunately, in my testing, I'm getting a lot of runtime failures, especially when compiling with DEBUG=TRUE. My best guess is that they're coming from the fix for #756 that was folded into this branch. I spot-checked two failures; these fail in different places but it's possible that the underlying cause is the same:
If possible, it would be great if, once those issues are thought to be resolved, someone could run at least the two tests given as examples above: to verify that they pass. Ideally, also running the full test suite on izumi would be a good addition before handing this back over for full testing: From the top level of the CTSM checkout on izumi, |
|
I'll take a look at this. |
|
I backed out the changes from the quadratic bug fix and the ERP_P36x2_D_Ld5.f10_f10_musgs.I2000Clm50Cn.cheyenne_gnu.clm-default test fails in the same place. Seems to be due to t_veg increasing over the course of the simulation from something reasonable to over 850K which results in an overflow due to the exponential. |
|
The problems appear to be associated with the replacement of these statements: with these statements: In particular the use of frac_sno here. The equation itself is valid, however, the frac_sno hyperbolic tangent formulation can result in values just slightly less than one despite a heavy snowcover, resulting in very small elai and esai (e.g., 3.1086060491376643E-020 and 2.2204460492503131E-016) that presumably are causing problems in the radiation code (e.g., fsun > 1 used in VOCEmissions and/or very high vegetation temperatures causing a problem with the exponential in Photosynthesis). The new equations are also present in SatellitePhenology, however, there is a restriction on elai there such that elai < 0.05 and esai < 0.05 are set to zero. At least this is my working hypothesis so far. |
|
That sounds like a reasonable hypothesis. Would it be possible to put in a
similar restriction on elai and esai in BGC mode.
…On Thu, Nov 5, 2020 at 3:11 PM Keith Oleson ***@***.***> wrote:
The problems appear to be associated with the replacement of these
statements:
elai(p) = max(tlai(p)*fb, 0.0_r8)
esai(p) = max(tsai(p)*fb, 0.0_r8)
with these statements:
elai(p) = max(tlai(p)*(1.0_r8 - frac_sno(c)) + tlai(p)*fb*frac_sno(c), 0.0_r8)
esai(p) = max(tsai(p)*(1.0_r8 - frac_sno(c)) + tsai(p)*fb*frac_sno(c), 0.0_r8)
In particular the use of frac_sno here. The equation itself is valid,
however, the frac_sno hyperbolic tangent formulation can result in values
just slightly less than one despite a heavy snowcover, resulting in very
small elai and esai (e.g., 3.1086060491376643E-020 and
2.2204460492503131E-016) that presumably are causing problems in the
radiation code (e.g., fsun > 1 used in VOCEmissions and/or very high
vegetation temperatures causing a problem with the exponential in
Photosynthesis).
The old expression depended solely on fb, which goes to 1 quickly.
The new equations are also present in SatellitePhenology, however, there
is a restriction on elai there such that elai < 0.05 and esai < 0.05 are
set to zero.
At least this is my working hypothesis so far.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFABYVF7REVM6MN6MSENZMTSOMPH7ANCNFSM4QFM6VOA>
.
|
|
I can test some thresholds. I think we would want the threshold to be as small as possible to avoid any possible killing off of vegetation that is trying to grow in BGC/CN mode, while obviously avoiding problems across the varied configurations we have in the model. |
|
Keith, you're good!
It's interesting to try and think about what the fractional snow cover
represents in this context? I can see how this subgrid parameterization
for snow covered fraction may interact in unexpected ways with the
vegetation and biogeochemistry code. For consistency, I might use the same
threshold in the BGC and SP configuration. Better yet, but this in as a
parameter in the parameter file (used in both codes) so we can test down
the road if needed.
…On Thu, Nov 5, 2020 at 3:40 PM Keith Oleson ***@***.***> wrote:
I can test some thresholds. I think we would want the threshold to be as
small as possible to avoid any possible killing off of vegetation that is
trying to grow in BGC/CN mode, while obviously avoiding problems across the
varied configurations we have in the model.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB5IWJCEUBJDTTERKHTC6MLSOMSVLANCNFSM4QFM6VOA>
.
|
|
Nice work, Keith! A thought that comes to mind: rather than trying to find an elai/esai threshold that walks the fine line between letting vegetation grow while not blowing things up, would it instead make sense to have some limit on frac_sno here? i.e., rather than using real(r8), parameter :: frac_sno_threshold = 0.999_r8 ! frac_sno values greater than this are treated as 1
if (frac_sno(c) <= frac_sno_threshold) then
frac_sno_adjusted = frac_sno(c)
else
! avoid tiny but non-zero elai and esai that can cause code to blow up
frac_sno_adjusted = 1._r8
end ifThen use The advantage I see there is that this lets elai and esai be just as small as before in the absence of snow cover, and only changes things when there is a big snow pack - so I'd hope this avoids the issue you're worried about of stopping vegetation from growing. |
|
Yes, I think I like your approach better. I think Will was alluding to this as well in terms of what frac_sno actually means in this context. |
|
I've implemented Bill's approach and these two tests now pass: ./create_test ERP_P36x2_D_Ld5.f10_f10_musgs.I2000Clm50Cn.cheyenne_gnu.clm-default I'm happy to try the izumi testing unless Danica would like to do that. |
|
Thanks for working through this Keith! The frac_snow threshold seems like a
good approach. Should we also include the same threshold in the SP code? I
wonder if this would also allow us to remove the ELAI thresholds in the SP
code.
I've not previously worked through any of the testing, but I'm happy to try
if that takes the burden off others.
…On Fri, Nov 6, 2020 at 9:40 AM Keith Oleson ***@***.***> wrote:
I've implemented Bill's approach and these two tests now pass:
./create_test
ERP_P36x2_D_Ld5.f10_f10_musgs.I2000Clm50Cn.cheyenne_gnu.clm-default
./create_test
SMS_D_Ld3.f10_f10_musgs.I2000Clm50BgcCru.cheyenne_intel.clm-default
I'm happy to try the izumi testing unless Danica would like to do that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGHW2QINKESYZXOGEBT4ZKDSOQRJNANCNFSM4QFM6VOA>
.
--
Dr. Danica Lombardozzi
she/her/hers
Terrestrial Sciences Section
Climate and Global Dynamics
National Center for Atmospheric Research
Boulder, CO 80305
email: dll@ucar.edu
office: (303) 497-1777
|
|
Looks like the testing on izumi has passed: ./cs.status.fails 1106-135044iz_int: 6 tests 1106-135044iz_nag: 35 tests 1106-135044iz_pgi: 2 tests ======================================================================== |
|
Thanks, @olyson ! I will run cheyenne testing and let people know how it goes.
|
|
Unless someone has boundless time and energy to test changing the SP parameterization to match BGC, I don't think this is necessarily critical to address at this point. @billsacks I'd say go ahead and merge. |
|
Yes, merging also sounds fine to me. I agree that it isn't clear why the
elai thresholds were added to the SP code, but they predate the snow burial
bug fix and may have some other functionality.
…On Sat, Nov 7, 2020 at 7:24 AM will wieder ***@***.***> wrote:
Unless someone has boundless time and energy to test changing the SP
parameterization to match BGC, I don't think this is necessarily critical
to address at this point. @billsacks <https://github.com/billsacks> I'd
say go ahead and merge.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGHW2QJSBW5LO2QBBSSYWIDSOVKALANCNFSM4QFM6VOA>
.
--
Dr. Danica Lombardozzi
she/her/hers
Terrestrial Sciences Section
Climate and Global Dynamics
National Center for Atmospheric Research
Boulder, CO 80305
email: dll@ucar.edu
office: (303) 497-1777
|
|
Thanks @wwieder and @danicalombardozzi . All tests pass now (thanks @olyson for your help getting there!); I will merge to master shortly. |
Description of changes
Grass/crop snow burial fraction fix
Specific notes
Updates grass and crop snow burial ('fb' in SatellitePhenologyMod.F90) from being 0.2m to change with PFT height and accounts for 20% lodging (falling over)
Contributors other than yourself, if any: David Lawrence, Bill Sacks
CTSM Issues Fixed (include github issue #):
Resolves #516
Resolves #756
Resolves #1116
Are answers expected to change (and if so in what way)?
Yes. Snow burial of all grass and crop PFTs will change, altering albedo and other related water and energy fluxes
Any User Interface Changes (namelist or namelist defaults changes)? No
Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on cheyenne for intel/gnu and izumi for intel/gnu/nag/pgi is the standard for tags on master)
This modification was used in: Lombardozzi, D. L., G. B. Bonan, W. Wieder, A. S. Grandy, C. Morris, and D. L. Lawrence (2018), Cover Crops May Cause Winter Warming in Snow-Covered Regions, Geophysical Research Letters, 45(18), 9889–9897
NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).