Arctic Carbon Cycle Updates #947
Conversation
There was a problem hiding this comment.
@lmbirch89 - thank you again for opening this pull request in order to share these improvements with us! I have a number of requests below, some small and some larger. I'm going to discuss some of the larger ones with some other folks to decide how we want to proceed, so keep an eye out for more from us soon.
Two over-arching questions we need to decide on are:
-
Should these changes be done in a backwards-compatible way, controlled by one or more namelist flags and pulling parameters out to file?
-
What should be done together, and what should be separated into separate PRs? This question is connected to the first: We generally try to separate answer-changing modifications from those that are introducing a new option but are answer-preserving for existing configurations. So, if we're going to make some of this non-answer-changing (via namelist flags), then that should be kept separate from any answer changes.
| ! | ||
| ! !LOCAL VARIABLES: | ||
| integer :: g,c,p !indices | ||
| integer :: fp !lake filter patch index | ||
| real(r8):: ws_flag !winter-summer solstice flag (0 or 1) | ||
| real(r8):: crit_onset_gdd !critical onset growing degree-day sum | ||
| real(r8):: crit_daylat !latitudinal light gradient in arctic-boreal | ||
| real(r8):: onset_thresh !flag onset threshold |
There was a problem hiding this comment.
It looks like you're using this effectively as a logical value. Please change it to a logical, such as:
logical :: do_onset ! flag for whether to do onset now(I realize that some other flags in this routine are real-valued like this... it's possible that's needed for some other reason, or possible that that shouldn't have been done that way in the past. In either case, let's keep new code cleaner in this respect.)
ekluzek
added
PR status: work in progress
priority: high
|
@lmbirch89 - First off, I want to thank you again for taking the time to share these changes with us. I talked about this today with @ekluzek @wwieder and others. Our plans / requests are:
Again, thank you for this! |
|
@lmbirch89 thanks for this contribution. I'll start reviewing this over the weekend and make some notes and ask questions. I'd suspect that having a call some time next week will also be helpful. Could you also pass along your talk from the LMWG, it may remind me of what all the modifications are intended to fix. |
|
@billsacks @wwieder I'm happy to chat further. I will email my presentation to you both from the working group meeting and fix some of these small things next couple days. |
|
@lmbirch89 can you attach your presentation to this PR, so we can all look at it, and it will remain as documentation? Thanks |
|
@ekluzek It says the file is too large. So any advice on that would be appreciated. |
|
Perhaps you could 'share' the presentation via google drive and then post
the link. Eventually, also, the meeting presentations will be posted to
the CESM website and could point to that.
…On Fri, Mar 27, 2020 at 3:58 PM lmbirch89 ***@***.***> wrote:
@ekluzek <https://github.com/ekluzek> It says the file is too large. So
any advice on that would be appreciated.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#947 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFABYVE2IY5HX33JRW4A6G3RJUOQVANCNFSM4LS3REHQ>
.
|
There was a problem hiding this comment.
It seems the modifications requested here generally fall into 3 categories:
- Phenology (esp. WRT onset and offset)
- LUNA, with several modifications to the application of LUNA for high latitudes
- Allocation parameters
The testing in high latitude systems looks very compelling, but it would be helpful to see the effects of these changes both on single point simulations that we tend to look at frequently (e.g. UMBS) and well as global runs (and especially under changing environmental conditions)
src/biogeochem/CNPhenologyMod.F90
Outdated
| onset_thresh=1.0_r8 | ||
| else if (onset_gddflag(p) == 1.0_r8 .and. t10(p) > SHR_CONST_TKFRZ & | ||
| .and. tmin10(p) > SHR_CONST_TKFRZ .and. ws_flag==1.0_r8 & | ||
| .and. dayl(g)>(crit_dayl/2.0_r8) .and. snow5d(c)<0.1_r8) then |
There was a problem hiding this comment.
I also wonder here if it makes sense to have this latitude threshold, or we'll end up with different dynamics in a global-scale simulation on either side 45 degrees? Would it be more clear to just define a new seasonally deciduous subroutine for the PFTs you're trying to target here? This would be for larches, arctic decid. shrubs and grasses, and temperate decid. trees? regardless, changing the parameterization of the later would likely need to be evaluated both at point scales (e.g. UMBS, 45.5 degrees N) and for global runs?
src/biogeochem/CNPhenologyMod.F90
Outdated
| onset_thresh=1.0_r8 | ||
| else if (onset_gddflag(p) == 1.0_r8 .and. t10(p) > SHR_CONST_TKFRZ & | ||
| .and. tmin10(p) > SHR_CONST_TKFRZ .and. ws_flag==1.0_r8 & | ||
| .and. dayl(g)>(crit_dayl/2.0_r8) .and. snow5d(c)<0.1_r8) then |
There was a problem hiding this comment.
Should this snow5d threshold be put on a parameter file? More generally, it means that plants can't leaf out until the snow is completely melted? Is this a good assumption for present day climate for taller vegetation? What are results of this assumption in changing climate scenarios?
There was a problem hiding this comment.
Yes, we should put the 0.1 value on the paramsfile as something like "snow5d_thresh_for_onset". Any other suggestions for a name?
src/biogeochem/CNPhenologyMod.F90
Outdated
| crit_daylat=54000-360*(65-grc%latdeg(g)) | ||
| if (crit_daylat < crit_dayl) then | ||
| crit_daylat = crit_dayl | ||
| end if |
There was a problem hiding this comment.
This will be another good one to have a test to see the implications for temperate systems and in global runs.
| @@ -907,13 +914,23 @@ subroutine CNSeasonDecidPhenology (num_soilp, filter_soilp , & | |||
| if (onset_gddflag(p) == 1.0_r8 .and. soilt > SHR_CONST_TKFRZ) then | |||
| onset_gdd(p) = onset_gdd(p) + (soilt-SHR_CONST_TKFRZ)*fracday | |||
| end if | |||
| !separate into Arctic boreal and lower latitudes | |||
There was a problem hiding this comment.
@ekluzek & @billsacks What's the best way to wrap this into a namelist flag to maintain backwards compatibility with CLM5 but also make sure the code is readable?
There was a problem hiding this comment.
- should we have this hardcoded behavior for latitude or define this by PFT?
There was a problem hiding this comment.
- the second part of this conditional with
onset_gddflag(p) == 1.0_r8seems to be asking for something different, but as implemented it also seems to be triggered in lower latitudes too, leading to higher than intended LAI and GPP
src/biogeochem/CNPhenologyMod.F90
Outdated
| onset_thresh=1.0_r8 | ||
| else if (onset_gddflag(p) == 1.0_r8 .and. t10(p) > SHR_CONST_TKFRZ & | ||
| .and. tmin10(p) > SHR_CONST_TKFRZ .and. ws_flag==1.0_r8 & | ||
| .and. dayl(g)>(crit_dayl/2.0_r8) .and. snow5d(c)<0.1_r8) then |
There was a problem hiding this comment.
What is this logic for crit_dayl/2.0_r8? The critical day length was previously used for offset, but now is being used for onset and divided by 2? The intent here is murky, but is this so onset doesn't happen too early in the growing season?
There was a problem hiding this comment.
Let's talk about this Wednesday.
There was a problem hiding this comment.
Looking forward to chatting about these, but this is just a day light threshold so phenology doesn't try to start when there is less than 6 hours of daylight.
There was a problem hiding this comment.
I've actually come back to this and realized that this is do backwards from Leah's intention. So I'll file this as a separate issue.
|
@lmbirch89 & @ekluzek there's lots to go over here. After you're able to digest a bit, would it be helpful to have a call to make a game plan moving forward? |
|
@lmbirch89 here's a document about adding namelist items to CTSM... https://wiki.ucar.edu/display/ccsm/Adding+New+Namelist+Items+to+CLM We should also move some of the hardcoded parameters you found to the paramsfile. I think there's enough examples in the code to "follow the recipe". But, let us know if you need more help than that. I think you should look a lot of this over, but mainly wait for our discussion on Wednesday to make the most progress. We might both change what we think we need to do, as well as be able to clarify any questions you have. |
src/biogeochem/CNPhenologyMod.F90
Outdated
| onset_thresh=1.0_r8 | ||
| else if (onset_gddflag(p) == 1.0_r8 .and. t10(p) > SHR_CONST_TKFRZ & | ||
| .and. tmin10(p) > SHR_CONST_TKFRZ .and. ws_flag==1.0_r8 & | ||
| .and. dayl(g)>(crit_dayl/2.0_r8) .and. snow5d(c)<0.1_r8) then |
There was a problem hiding this comment.
Since this is becoming complex we should probably make this into its own little function. Also the "45.0" degree threshold should be a parameter on the paramsfile.
src/biogeochem/CNPhenologyMod.F90
Outdated
| crit_daylat=54000-360*(65-grc%latdeg(g)) | ||
| if (crit_daylat < crit_dayl) then | ||
| crit_daylat = crit_dayl | ||
| end if |
There was a problem hiding this comment.
54000, 360, and 65 should become parameters on the paramsfile. Do any of these relate to other parameters? And what are some suggestions on names?
|
|
||
| vcmaxse = 668.39_r8 - 1.07_r8 * min(max((t10(p)-tfrz),11._r8),35._r8) | ||
| jmaxse = 659.70_r8 - 0.75_r8 * min(max((t10(p)-tfrz),11._r8),35._r8) | ||
| vcmaxse = 486.0_r8 |
There was a problem hiding this comment.
If these become hardcoded values, we should put them on the params file. And possibly the old hard coded constants should as well, and then a namelist trigger between the two.
There was a problem hiding this comment.
This is also part of a use_Leuning switch, if it's made
|
Namelist control for Luna can go into the "luna" namelist in LunaMod.F90. Namelist control can go into "cnphenology" namelist in cnphenology. Photosynthesis doens't have a namelist, but we could add one to it. It does have an Init method. I think the changes in TemperatureMod and WaterDaignosticBulkType are just to add accumulation variables. So these could be done all the time whether they are needed or not (or use the luna or chphenology namelist triggers to determine if they are on or no. |
In the past we have talked about preferring if accumulation variables are owned by particular parameterizations - so these variables would live in derived types specific to those parameterizations. This keeps logic more localized and makes it more straightforward to exclude them if the given parameterization is inactive. However, I don't feel strongly on this point. |
That would be preferred so that it didn't impact parts of the code outside the area that's really being changed. That would also make the accumulation naturally only happen when the new code is invoked. So that it is much cleaner and understandable. |
|
@dlawrenncar and @wwieder should the way this be handled is a namelist toggle to choose between Leuning and Kattge? And are there other options that should be added in as well? And perhaps we need to do our own simulations with Leuning to test its global response? @danicalombardozzi on Wednesday tell us more about what Nick Smith is doing and if his work is ready to come in or not. |
|
Given @danicalombardozzi 's comments, I think we want to default to Kattge. For simulations done by @lmbirch89, it seems like Leuning was used. Down the road, it would be great to bring in Nick's ideas, but as far as I know these are not ready for game time right now. With the aim of bringing this in for the parameter perturbation experiment, @dlawrenncar it seems like we'd want Kattge for sure. Let's talk Weds. |
|
Okay, I have force pushed a few commits that fix the history. Note that the update (from hash 32d0854 to hash dd0cbec) doesn't change the end result at all, but fixes the history. @lmbirch89 - a full answer to your question is complicated (and dinner is calling, so I've gotta run), but the short version is: while the end result was correct, merging on top of an incorrect state didn't fix the history of the CNPhenologyMod file. For example, changes to that file that I made became attributed to you instead. That's fixed now. |
|
Hi @lmbirch89, I wanted to circle back to see where you were with the requests made above and the long conversations about the code and parameter modifications made here? It seems like a bunch of this has been addressed, or is mostly there? I'm asking because we'd like to do a clean comparison of a series of clean 2 degree simulations with:
I think initially we may stick with the Kattge temperature response functions with acclimation (not Leuning). @olyson is going to spinup 1 & 2 now with the aim of running transient 20th century runs that can be compared to ILAMB. We'd like to do the same with 3, when you feel like the code is close enough for this kind of test. @ekluzek can make a branch that reverts to Kattge for now, for these testing purposes. |
|
@wwieder I've cleaned up everything so it runs as I expect at the flux tower sites, and I'm finalizing if the gridded global output in this version of CLM matches my previous regional simulations (when I didn't have a latitudinal threshold for onset, since it didn't matter). Short answer is I think I addressed everything that was asked of me (except the boolean for onset_thresh instead of 0 or 1). I can confirm I'm happy with the global simulation over this weekend though. |
OK, it sounds like we should wait on this before we make our branch for our testing. If you can run over the weekend, and then let us know whether it looks right or not, that would be helpful. Then I can make a branch that reverts Leuning to Kattge and we can do our testing with that. |
|
I have gone through my own comments and requests to see what has and hasn't been addressed at this point. To help keep track of these requests, I am gathering the remaining outstanding points in a checklist here. Most of these are code cleanup, but a few are more about the science. I'll periodically update this comment as I see that things have been addressed. Note that this just includes comments that I made - not comments made by @wwieder @ekluzek or others.
|
|
Bll I may need a how to you actually keep track of things on github
tutorial... not an item for a friday afternoon.
…On Fri, Apr 17, 2020 at 12:01 PM Bill Sacks ***@***.***> wrote:
I have gone through my own comments and requests to see what has and
hasn't been addressed at this point. To help keep track of these requests,
I am gathering the remaining outstanding points in a checklist here. Most
of these are code cleanup, but a few are more about the science. I'll
periodically update this comment as I see that things have been addressed. *Note
that this just includes comments that I made - not comments made by
@wwieder <https://github.com/wwieder> @ekluzek <https://github.com/ekluzek>
or others.*
-
#947 (comment)
<#947 (comment)> -
Confirming logic for the latitude threshold in the onset trigger. My
comment was really aimed at whether low latitudes are supposed to only use
the old trigger, or if (as the code is currently written) they are supposed
to use either the old or new triggers. This comment may become irrelevant
if the logic is reworked to avoid use of the latitude threshold.
-
#947 (comment)
<#947 (comment)> and
the following discussion - Reuse of crit_dayl in a questionable way
(as opposed to adding a new parameter)
-
#947 (comment)
<#947 (comment)> and
the following discussion - Adding new parameters governing the
latitude-dependent function
-
#947 (comment)
<#947 (comment)> and
the following discussion - Kattge vs. Leuning
-
#947 (comment)
<#947 (comment)> and
the following discussion - Hard-coded soil level 3
-
#947 (comment)
<#947 (comment)> and #947
(comment)
<#947 (comment)> -
Changing references in comments
-
#947 (comment)
<#947 (comment)> -
soila10_patch should be changed to column-level
-
#947 (comment)
<#947 (comment)> -
Unnecessary use of rbufslp temporary variable
-
Changing onset_thresh into a logical, do_onset
-
#947 (comment)
<#947 (comment)> - Line
that can be deleted
-
May need namelist flags for backwards compatibility
-
May want to consider reverting the bug fixes that are now in a
different PR
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#947 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB5IWJB6N6HWE6CORWZMJ4LRNCKPFANCNFSM4LS3REHQ>
.
--
Will Wieder
Project Scientist
CGD, NCAR
303-497-1352
|
Through lots of sweat, blood and tedium. (Okay, maybe not blood... and often not too much sweat either... but definitely tedium, plenty of tedium.) |
|
@billsacks @wwieder I also really wanted to ask and know the answer to that question, haha. Thanks so much Bill! |
|
A bit less tongue-in-cheek, the best I've been able to figure out is hitting the "resolve" button on comments that have been fully resolved (note: we prefer that this marking as resolved be done by the original commenter of a given comment / thread), and then, once a PR gets an overwhelming number of comments (like this one has), starting a one-stop-shopping checklist of the outstanding items that I can edit over time. I periodically feel like there must be a better way, but I haven't found one yet. |
|
(It doesn't help that GitHub hides some comments when there get to be too many: I often need to search for "hidden" or "load more" on the page to make sure I have found everything.) |
|
OK, I'll start going through and making my own checklist that hopefully doesn't overlap with @billsacks.
Here are some other minor issues to correct
|
|
@lmbirch89 did you have a chance to look at your global runs to see if they look sensible? |
|
@wwieder Yes, sorry they are done running and look at first glance acceptable, but I just needed to check a couple pft specific things. I can do it tonight. |
|
Thanks @lmbirch89 if these look OK let us know. Then @ekluzek will create a branch for @olyson to run a clean spinup to compare default CLM5, LunaBug fixes, and the science changes you're suggesting (without Leunning to start with). |
|
@wwieder @ekluzek @olyson With all my changes implemented (not the clean up suggested above yet), the global results are what I expect at high latitudes and seem reasonable everywhere else too. The red line in the seasonal GPP figures is the global model development run with 2 degree resolution. I am still running with Leuning in the figures below. (I can share more figures if necessary, but I thought this was a nice quick summary.) |
|
Great news, @lmbirch89! We'll get started on running our own tests here. Can you keep updating this PR to address the remaining issues we've highlighted. @ekluzek does @lmbirch89 need to back out all off the LUNAbugs from this PR (this is something both @billsacks and I flagged)? p.s. that annual cycle is unreal! |
ekluzek
removed
the
priority: high
ekluzek
added
the
closed: wontfix
|
This one has been converted into the luna bug fixes that already came in, and #990 that is this one with Kattge rather than Leuning. |
Description of changes
We have addressed the issues in phenology and photosynthesis in the high latitudes. Development was focused on PFT specific differences and we used observations to inform model development. GPP is improved now such that the tundra is realistically less productive than the boreal forest.
Specific notes
List of Improvements to Arctic-Boreal
Mechanistic Onset/Offset (Phenology)
Onset=soil and air temperatures above freezing
Offset=latitudinal gradient
Maximum Day Length Scaling for Jmax (Luna and photosynthesis)
Leuning Temperature Scaling Parameters, removing the 11 to 35C threshold (Luna and photosynthesis)
Small bug fix where day temperature was used instead of night (Luna)
Dynamic Stem Leaf Allocation (parameter file)
Increased Root Allocation for Grasses/Shrubs (parameter file)
Increased Leaf Allocation for Deciduous Trees (parameter file)
Contributors other than yourself, if any: Brendan Rogers and Danica Lombardozzi have helped with this and work was done as part of NASA's Carbon Cycle Science and ABoVE.
CTSM Issues Fixed (include github issue #): High GPP bias
Are answers expected to change (and if so in what way)?
Yes, the high bias of GPP and LAI has been reduced in the ABZ.
Any User Interface Changes (namelist or namelist defaults changes)?
I have a new parameters file in /glade/work/lbirch/devclm4git/clm5_params_arcticupdate.nc
Testing performed, if any:
A paper is being submitted to GMD of everything we worked through. We used comparisons with Fluxcom, flux towers, and ILAMB. This has also been presented a few times to the NCAR land model working group.
ilamb_scoreall.pdf
griddedclm5_gpp.pdf
clm_gridflux.pdf
A global simulation also ran successfully with changes contained to the high latitudes, except for the Leuning 2003 temperature scaling, which had been used in a previous version of CLM.
(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)
NOTE: Be sure to check your Coding style against the standard:
https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines
This change is