Refactor of patch and cohort objects

[adrifoster]

First refactor of patch and cohort objects to facilitate unit testing and decrease circular dependencies

Description:

I moved the patch and cohort objects into their own modules, and moved some procedures in FatesCohortDynamicsMod and FatesPatchDynamicsMod into those modes as bound procedures associated with each type.

This also necessitated some modifications of other modules/types to get rid of circular dependencies.

Most notably, I got rid of the pointers from the cohort type to their respective patch. I did not think this was necessary, and created circular dependencies (the pointer from each patch to its cohort linked list remains). Where information about the patch was necessary, I converted the procedures to take in patch-specific variables/attributes rather than the entire patch structure itself.

To be coordinated with ESCOMP/CTSM#2000 and E3SM-Project/E3SM#5849

Collaborators:

@rgknox ; @billsacks ; @glemieux ; @JessicaNeedham

Expectation of Answer Changes:

None - all tests should be B4B

Checklist:

• My change requires a change to the documentation.
• I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
• I have read the CONTRIBUTING document.
• FATES PASS/FAIL regression tests were run
• If answers were expected to change, evaluation was performed and provided

Test Results:

see /glade/scratch/afoster/tests_fates_refactor_latest

all tests are PASS or EXPECTED FAIL

CTSM (or) E3SM (specify which) test hash-tag: ctsm5.1.dev123-14-g2e8d71075

CTSM (or) E3SM (specify which) baseline hash-tag: ctsm5.1.dev123

FATES baseline hash-tag: sci.1.65.4_api.25.4.0

Test Output:

fates_refactor_latest_gnu: 3 tests
    FAIL ERS_Lm13.f10_f10_mg37.I2000Clm50Fates.cheyenne_gnu.clm-FatesCold COMPARE_base_rest (EXPECTED FAILURE)

 
fates_refactor_latest_int: 33 tests
    PEND ERP_P72x2_Ld30.f45_f45_mg37.I2000Clm50FatesRs.cheyenne_intel.clm-mimicsFatesCold SHAREDLIB_BUILD (EXPECTED FAILURE)
    PASS ERS_D_Ld5.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesColdHydro COMPARE_base_rest (UNEXPECTED: expected FAIL)
    FAIL ERS_Lm12.1x1_brazil.I2000Clm50FatesCruRsGs.cheyenne_intel.clm-FatesFireLightningPopDens COMPARE_base_rest (EXPECTED FAILURE)
    FAIL ERS_Lm13.f45_f45_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdNoComp COMPARE_base_rest (EXPECTED FAILURE)

[glemieux]

A few clean up suggestions and a couple of questions. Sorry for the delay in reviewing!

[glemieux]

@adrifoster found during regression testing late last week that the fates spmode tests where not B4B against the baseline. The culprit appears to have been that in the call to create_cohort at the end of init_cohorts the refactor was passing EDPftvarcon_inst%hgt_min(pft) as the input height argument for the new cohort during all run modes. Currently on main, we set temp_cohort%hite directly to either 0.5_r8 or EDPftvarcon_inst%hgt_min(pft) depending on if we're in sp mode or note. This temp_cohort%hite value is then passed into create_cohort via argument.

Cheyenne is currently down, so I can't run the fix in commit cf41e0a against our typical fates suite regression tests, but I did run this on lobata comparing the output of a one year long sp mode smoke test at BCI against main using cprnc and the results were B4B, so I'm pretty confident this is the fix.

As an aside, this raises the question: for initialization only, is using the hgt_min for the given pft a decent enough usage for initializing cohorts during sp mode as well? Thoughts @rosiealice?

[glemieux]

It looks like the issue that I identified is only part of the story: global sp mode tests are still failing against the baseline. That said, the 1x1_brazil smoke test passes b4b as expected (given that my test for the fix was for a single site).

Test results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1024-spfix

UPDATE: Running a daily case, it looks like this is happening on the second time step, so is probably not an initialization issue. Plotting out the DIFF results show no particular global pattern.

[glemieux]

It looks like the thing that was causing the non-b4b results against the baseline for the global sp mode runs was the additional call of leafc_from_treelai. The refactor had a call to this function inside the new calculate_SP_properties and then again inside assign_cohort_sp... after the calculate_SP_properties call. I've removed the additional call via a6c3a74.

This has me thinking that we were seeing as not-b4b prior to my commit is actually more correct. Right now on main, we adjust cohort%c_area and cohort%n after the call to leafc_from_treelai (as necessary), which uses the former cohort values as input:

fates/biogeochem/EDPhysiologyMod.F90

Lines 1952 to 1979 in caf4aae

	leaf_c = leafc_from_treelai( currentCohort%treelai, currentCohort%pft, currentCohort%c_area,&
	currentCohort%n, currentCohort%canopy_layer, currentCohort%vcmax25top)
	!check that the inverse calculation of leafc from treelai is the same as the
	! standard calculation of treelai from leafc. Maybe can delete eventually?
	check_treelai = tree_lai(leaf_c, currentCohort%pft, currentCohort%c_area, &
	currentCohort%n, currentCohort%canopy_layer, &
	canopylai,currentCohort%vcmax25top )
	if( abs(currentCohort%treelai-check_treelai).gt.1.0e-12)then !this is not as precise as nearzero
	write(fates_log(),*) 'error in validate treelai',currentCohort%treelai,check_treelai,currentCohort%treelai-check_treelai
	write(fates_log(),*) 'tree_lai inputs: ', currentCohort%pft, currentCohort%c_area, currentCohort%n, &
	currentCohort%canopy_layer, currentCohort%vcmax25top
	call endrun(msg=errMsg(sourcefile, __LINE__))
	end if
	! the carea_allom routine sometimes generates precision-tolerance level errors in the canopy area
	! these mean that the canopy area does not exactly add up to the patch area, which causes chaos in
	! the radiation routines. Correct both the area and the 'n' to remove error, and don't use
	!! carea_allom in SP mode after this point.
	if(abs(currentCohort%c_area-parea).gt.nearzero)then ! there is an error
	if(abs(currentCohort%c_area-parea).lt.10.e-9)then !correct this if it's a very small error
	oldcarea = currentCohort%c_area
	!generate new cohort area
	currentCohort%c_area = currentCohort%c_area - (currentCohort%c_area- parea)
	currentCohort%n = currentCohort%n * (currentCohort%c_area/oldcarea)

@rosiealice given the dependency of leaf carbon on cohort area and number, should we actually be waiting to call leafc_from_treelai after the adjustments to cohort area and number?

[glemieux]

Generating a new fates baseline using ctsm5.1.dev132. I'll rerun the full fates test suite against this later tonight. If that looks good, I'll kick off aux_clm tests as well.

[rosiealice]

Right, I see what you mean. I agree it seems like there is no reason to call the leafc_dfrom_treelai before the error correction of the canopy area here...
This was all a very strange piece of precision tolerance wrangling. I feel like it would all work much better if the tolerances had some inbuilt bit of canopy gap from which tiny errors could be added and subtracted without it causing havoc. @rgknox this might be easier with the 'ghost element' in the new two stream code is suspect?

[rosiealice]

Also responding to the hgt_min question from earlier in this thread, does this mean that the intitializatio value will only operate for perhaps the very first time step (or not even that) before being overwritten with the values from the surface dataset? If so then that is fine...

[glemieux]

Regression testing on cheyenne against fates-sci.1.67.0_api.25.5.0-ctsm5.1.dev132 is complete and all expected tests are B4B. Testing on izumi and aux_clm suite is in progress.

UPDATE: fates suite testing on izumi is b4b for all expected tests against the baseline.

Cheyenne location: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1024-leaf_c_fix
Izumi location: /home/glemieux/scratch/ctsm-tests/tests_pr1024

[glemieux]

Also responding to the hgt_min question from earlier in this thread, does this mean that the intitializatio value will only operate for perhaps the very first time step (or not even that) before being overwritten with the values from the surface dataset? If so then that is fine...

That's correct @rosiealice, it would only be during the init_cohorts subroutine prior to the first call of satellite_phenology.