Fates Fixed Biogeography #632

rosiealice · 2020-04-20T09:11:46Z

This PR updates the FATES side to implement the transfer of the PFT area fields into FATES, and prevents plants establishing outside of areas where they are not specified in the surface dataset. The PFTs still compete, hence this is Fixed Biogeography WITH competition

Description:

This corresponds to this item in the reduced complexity modes project. https://github.com/NGEET/fates/projects/5#card-36625297

THIS PR SHOULD BE ADOPTED AT THE SAME TIME AS THE CORRESPONDING FATES_API PR ESCOMP/CTSM#985

Collaborators:

@rgknox , @ckoven , @huitang-earth @glemieux @ekluzek

Expectation of Answer Changes:

This will change answers, because all PFTs cannot live everywhere
BUT only if the new use_fates_fixed_biogeog switch is turned ON
(thus might need an extra test...)
I didn't test answer changes for when it's off...

Checklist:

My change requires a change to the documentation.
I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
[ x] I have read the CONTRIBUTING document.
FATES PASS/FAIL regression tests were run
[x ] If answers were expected to change, evaluation was performed and provided
Test run output on cheyenne here:
/glade/scratch/rfisher/archive/clm5-spfates_test/lnd/hist
running from

/glade/work/rfisher/git/ctsm_apr20/cime/scripts/clm5-spfates_test

Test Results:

CTSM (or) E3SM (specify which) test hash-tag: rosiealice/ctsm@ad2ecaa

CTSM (or) E3SM (specify which) baseline hash-tag: ESCOMP/CTSM@d525994

FATES baseline hash-tag: 2d98623

Test Output:

All expected PASS

/glade/u/home/glemieux/scratch/clmed-tests/PR632-CTSM985-premerge-finalcheck-Cad2ecaae-F0f2ea162.fates.cheyenne.intel
/glade/u/home/glemieux/scratch/clmed-tests/PR632-CTSM985-premerge-finalcheck-Cad2ecaae-F0f2ea162.fates.cheyenne.gnu

ckoven

Looks good to me, thanks @rosiealice!

ckoven · 2020-04-20T23:05:18Z

biogeochem/EDPhysiologyMod.F90

             ! Seed input from local sources (within site)
             litt%seed_in_local(pft) = litt%seed_in_local(pft) + site_seed_rain(pft)/area

             ! Seed input from external sources (user param seed rain, or dispersal model)
             seed_in_external =  seed_stoich*EDPftvarcon_inst%seed_suppl(pft)*years_per_day
-
+             if(hlm_use_fixed_biogeog.eq.itrue)then 


should this be: ?

Suggested change

if(hlm_use_fixed_biogeog.eq.itrue)then

if ( hlm_use_fixed_biogeog .eq. itrue .and. currentSite%use_this_pft(pft) .eq. ifalse ) then

yeah, I agree, @ckoven , it does look like that should have the PFT filter.

So, this is the part that breaks the carbon balance at restart. In particular, we aren't actually setting any of the use_this_pft or area_pfts after the model has restarted. Fixing this is the first thing on my to-do list after this set of changes. I think it's best to have both the variables on the restart file, to be consistant with how everything else about the site is set up, rather than recalculating them from the bc_in variable. Yes? No?

It sounds like, following a restart, this routine is being called before use_this_pft is getting filled from the bc_in structure. I think it would probably be a good idea to put the use_this_pft filter in the restart, as its memory overhead is small compared to our cohort variables.

On which note, I'm following 'recrate' around as an example of a similarly-dimensioned variable to figure out how to put it in the restart file. I am confused about why it is registered as a cohort variable here...

fates/main/FatesRestartInterfaceMod.F90

Line 1003 in 82691fb

call this%set_restart_var(vname='fates_recrate', vtype=cohort_r8, &

Also, as a matter of general code development strategies, would you typically make a new branch to add such a thing to an open PR, or just push more commits to the same branch?

This is just my strategy, but 95% of the time I just push new commits to the same branch. Sometimes I will pack a set of changes into a new branch and use a PR to update the branch with the original pull request, but I only do that usually in two scenarios, 1) to make it clear to reviewers what I changed per their requests, and only if the changes are significant, or 2) if I plan to make a large set of complicated changes to the original feature branch, and want a quick and easy way to abort if I totally mess things up.

ckoven · 2020-04-20T23:05:29Z

biogeochem/EDPhysiologyMod.F90


+    do ft = 1,numpft
+      if(currentSite%use_this_pft(ft).eq.1)then


Suggested change

if(currentSite%use_this_pft(ft).eq.1)then

if(currentSite%use_this_pft(ft).eq.itrue)then

ckoven · 2020-04-20T23:05:41Z

main/EDInitMod.F90

+          end if
+
+          do ft = 1,numpft
+           sites(s)%use_this_pft(ft) = 1


Suggested change

sites(s)%use_this_pft(ft) = 1

sites(s)%use_this_pft(ft) = itrue

ckoven · 2020-04-20T23:05:45Z

main/EDInitMod.F90

+           sites(s)%use_this_pft(ft) = 1
+           if(hlm_use_fixed_biogeog.eq.itrue)then
+             if(sites(s)%area_pft(ft).gt.0.0_r8)then
+                sites(s)%use_this_pft(ft) = 1


Suggested change

sites(s)%use_this_pft(ft) = 1

sites(s)%use_this_pft(ft) = itrue

ckoven · 2020-04-20T23:05:52Z

main/EDInitMod.F90

+             if(sites(s)%area_pft(ft).gt.0.0_r8)then
+                sites(s)%use_this_pft(ft) = 1
+             else
+                sites(s)%use_this_pft(ft) = 0


Suggested change

sites(s)%use_this_pft(ft) = 0

sites(s)%use_this_pft(ft) = ifalse

ckoven · 2020-04-20T23:05:54Z

main/EDInitMod.F90

@@ -464,7 +488,7 @@ subroutine init_cohorts( site_in, patch_in, bc_in)
    patch_in%shortest => null()

    do pft =  1,numpft
-
+     if(site_in%use_this_pft(pft).eq.1)then


Suggested change

if(site_in%use_this_pft(pft).eq.1)then

if(site_in%use_this_pft(pft).eq.itrue)then

rgknox · 2020-04-21T20:22:34Z

main/EDInitMod.F90

@@ -216,10 +220,12 @@ subroutine zero_site( site_in )
    ! canopy spread
    site_in%spread = 0._r8

+    site_in%area_pft(:) = 0._r8
+    site_in%use_this_pft(:) = fates_unset_int


This is great, I like initializing things to unset.
Also, area_pft looks like it could be unset also.

I used 'fates_unset_int' on the use_this_pft as it's an integer, but all the other real numbers above were set to 0._r8 so that seems to be the protocol there. Is there an unset for real numbers?

yup, fates_unset_r8, defined after fates_unset_int, see here: https://github.com/NGEET/fates/blob/master/main/FatesConstantsMod.F90#L22

rgknox · 2020-04-21T20:26:10Z

@rosiealice , I was curious about future plans for the site%area_pft array.
So far, it only looks like it is being used to determine if a pft is used or not used. In later versions we are going to be using this to create patches with specific fractional areas, or other uses right?

rosiealice · 2020-04-24T15:56:51Z

@rgknox on your query bout area_pft, yes, indeed. When we start to use it for the no competition FBG mode, it'll have more of a role to play.. And also for SP mode.

rosiealice · 2020-05-05T07:49:59Z

I think this might be good to go now. One the call last week we had talked about adding a test, and @rgknox mentioned that I should look at Jessie's PR for age-related mortality. #599 Having looked at that, I noticed that she had some sort of internal test

fates/biogeochem/EDMortalityFunctionsMod.F90

Line 196 in 0059286

if (test_zero_mortality) then

but I couldn't find and reference to a software test of the use_cohort_age_tracking facility in the test suite, which was the kind of test I was worrying about (for the use_fixed_biogeog facility introduced here). Should I make the test suite test? If so, can someone point me to an example thereof). If not, I think this part is doneas far as I can tell.

glemieux · 2020-05-05T14:59:26Z

@rosiealice I think the test that @rgknox was referencing may have been in @JessicaNeedham's concurrent CTSM pull request ESCOMP/CTSM#882. I think this is the relevant link: ESCOMP/CTSM@ae551ed.

…d in deconflict

…g trampled in merge deconflict

…ng merge

Deconflict PR 632 with FATES master (commit 8abef14)

glemieux · 2020-06-05T19:15:15Z

Final regression test ran with comparison against fates-sci.1.35.4_api.11.0.0. All expected PASS:

/glade/u/home/glemieux/scratch/clmed-tests/PR632-CTSM985-premerge-finalcheck-Cad2ecaae-F0f2ea162.fates.cheyenne.intel
/glade/u/home/glemieux/scratch/clmed-tests/PR632-CTSM985-premerge-finalcheck-Cad2ecaae-F0f2ea162.fates.cheyenne.gnu

rosiealice added 9 commits April 8, 2020 10:56

basic SP modifications. compiles

17971e9

adding SBG fix

dcc8e58

added filters on recruitment

2d015c8

mask out recruitment terms

edc9c14

added bc_in variable

7c35d61

adding bc_in call to set_sites_properties. Ties into changes in HLM

34cd0af

adding bc_in call to set_sites_properties. Ties into changes in HLM

0689d95

changed name of static_biogeog to fates_biogeog

020bd97

fixed restart bug. phew

05ac748

rosiealice mentioned this pull request Apr 20, 2020

Fixed Biogeeography: FATES_NEXT_API ESCOMP/CTSM#985

Merged

rosiealice added 2 commits April 20, 2020 03:39

adding use_fates_nocomp switch

2f506cc

turning off print cohorts

7a38199

rosiealice added the enhancement label Apr 20, 2020

ckoven approved these changes Apr 20, 2020

View reviewed changes

rgknox reviewed Apr 21, 2020

View reviewed changes

rgknox assigned glemieux Apr 22, 2020

rosiealice added 5 commits April 24, 2020 10:03

changes to use itrue and ifalse

c0ceb91

Merge branch 'fates_sp' into fates_nocomp

cb3fe0b

adding restart capability for use_this_pft

e39ad3f

typo in EDphys

691f4cf

roll back snow change

347270b

rosiealice mentioned this pull request Apr 27, 2020

Inconsistency between PCT_NAT_PFT and fates_pftname in E3SM-FATES #634

Open

glemieux mentioned this pull request May 19, 2020

Deconflict CTSM PR 985 with fates_next_api (commit d525994) rosiealice/ctsm#2

Merged

Merge branch 'ngeet-master' into fates_sp-deconflict-8abef14

8364ff8

glemieux added 2 commits May 19, 2020 11:40

making set_bcs public as it was missed in PR647

9876102

removing dead code from PR647

87de446

glemieux mentioned this pull request May 19, 2020

Deconflict PR 632 with FATES master (commit 8abef14) rosiealice/fates#2

Merged

5 tasks

glemieux and others added 6 commits May 19, 2020 13:56

reinstating pft_areafrac to FatesInterfaceTypeMod after it was delete…

9b7f66c

…d in deconflict

reinstating hlm_use_fixed_biogeog to FatesInterfaceTypeMod after bein…

b235e96

…g trampled in merge deconflict

commenting out nocomp for future reduced complexity mode

66c2cc8

commenting out what looks like future code

111e24c

reinstating hlm_use_fixed_biogeog to EDInitMod that got trampled duri…

0ffab8b

…ng merge

Merge pull request #2 from glemieux/fates_sp-deconflict-8abef14

0f2ea16

Deconflict PR 632 with FATES master (commit 8abef14)

glemieux merged commit 7c065e2 into NGEET:master Jun 5, 2020

ckoven mentioned this pull request Jun 13, 2020

Use HLM-provided land-use time series data to drive FATES harvest #663

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fates Fixed Biogeography #632

Fates Fixed Biogeography #632

rosiealice commented Apr 20, 2020 •

edited by glemieux

Loading

ckoven left a comment

ckoven Apr 20, 2020

rgknox Apr 21, 2020

rosiealice Apr 24, 2020

rgknox Apr 24, 2020

rosiealice Apr 24, 2020

rosiealice Apr 24, 2020

rgknox Apr 24, 2020

ckoven Apr 20, 2020

ckoven Apr 20, 2020

ckoven Apr 20, 2020

ckoven Apr 20, 2020

ckoven Apr 20, 2020

rgknox Apr 21, 2020

rosiealice Apr 24, 2020

rgknox Apr 24, 2020 •

edited

Loading

rgknox commented Apr 21, 2020

rosiealice commented Apr 24, 2020 •

edited

Loading

rosiealice commented May 5, 2020

glemieux commented May 5, 2020

glemieux commented Jun 5, 2020

	if(hlm_use_fixed_biogeog.eq.itrue)then
	if ( hlm_use_fixed_biogeog .eq. itrue .and. currentSite%use_this_pft(pft) .eq. ifalse ) then

	if(currentSite%use_this_pft(ft).eq.1)then
	if(currentSite%use_this_pft(ft).eq.itrue)then

	sites(s)%use_this_pft(ft) = 1
	sites(s)%use_this_pft(ft) = itrue

	sites(s)%use_this_pft(ft) = 0
	sites(s)%use_this_pft(ft) = ifalse

	if(site_in%use_this_pft(pft).eq.1)then
	if(site_in%use_this_pft(pft).eq.itrue)then

Fates Fixed Biogeography #632

Fates Fixed Biogeography #632

Conversation

rosiealice commented Apr 20, 2020 • edited by glemieux Loading

Description:

Collaborators:

Expectation of Answer Changes:

Checklist:

Test Results:

ckoven 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

rgknox Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

rgknox commented Apr 21, 2020

rosiealice commented Apr 24, 2020 • edited Loading

rosiealice commented May 5, 2020

glemieux commented May 5, 2020

glemieux commented Jun 5, 2020

rosiealice commented Apr 20, 2020 •

edited by glemieux

Loading

rgknox Apr 24, 2020 •

edited

Loading

rosiealice commented Apr 24, 2020 •

edited

Loading