cleaning up nocomp patch indexing

[rgknox]

Description:

This set of changes removes the need for all these checks that determine if a patch is nocomp+fixedbiogeog. We currently do this check because when in this mode, we construct a bareground patch in the zero-th patch index position. When we traverse patch loops, sometimes we increment the patch index, and this logic check makes sure that we don't increment.

However, we don't need to put this special bare-ground patch in the zeroth position. We can just put it in the first position, and allow it to be in-line index-wise with the what ELM/CLM identifies as the 1st vegetated patch. This is fine, FATES has patches in the full dynamics mode that have no cohorts, it's used to having patches with no vegetation lined-up with patches in CLM/ELM that are "vegetated". What happens is we set the weighting to zero, and many of the fluxes are some nominal value.

We also don't need to touch the ELM/ELM side of the code, because the way we create out weightings actaully works for both the indexing schemes described. Why? because we assign the CLM/ELM bare-ground patch fraction based on 1 minus the sum of all patches total canopy area, which doesn't care about index position.

Here are some slides that give a sense of what is going on and what is proposed: https://docs.google.com/presentation/d/1ZwErNVF3B-_jJwpCDud3d-MtR59Idax8YTWe_ErEc8Q/edit?usp=sharing

Further, if someone want's to bypass some subset of physics, or some algorithm, because there is no vegetation, it would be more appropriate anyway to compare against total canopy area, and not if it is a nocomp+bareground patch.

I still need to make sure that we don't exceed our patch counts on the fates-side when in nocomp+fixedbiogeog, in this mode we were using 1 less space as we matched with ELM/CLM arrays.

Collaborators:

Discussions with @glemieux

Expectation of Answer Changes:

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

Contributor

• The in-code documentation has been updated with descriptive comments
• The documentation has been assessed to determine if updates are necessary

Integrator

• FATES PASS/FAIL regression tests were run
• Evaluation of test results for answer changes was performed and results provided

Documentation

• Technical Note update:
• User's Guide update:

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

[glemieux]

We also don't need to touch the ELM/ELM side of the code, because the way we create out weightings actaully works for both the indexing schemes described.

For the hlm-fates interface mod, we skip the hlm bareground patch, col%patchi(c), by mapping fates patch index ifp onto hlm patch index p via p = ifp+col%patchi(c). If we make ifp=1 the bareground patch for fates, wouldn't we want to remove this mapping on the hlm side API?

[glemieux]

@rgknox and @glemieux to talk about this this coming Thursday.

[rgknox]

FATES test suite is generating some unexpected fails and diffs with base.

The coldbasic satphen tests show differences in 'PCT_NAT_PFT'
Another satphen tests has many diffs in many variables..
There appears to be a problem building or executing two of the luh enabled tests
One of the fixedbiogeog tests had diffs only in FATES_PATCHAREA_LU and FATES_DISTURBANCE_RATE_MATRIX_LULU

I think some of this has to do with fire and how we decide which patch to use for input boundary conditions on the site scale?

Either way, plenty to fix here.

./cs.status.fails
1210-094439de_gnu: 10 tests
    FAIL ERS_D_Ld15.f45_f45_mg37.I2000Clm50FatesRs.derecho_gnu.clm-FatesColdTwoStreamNoCompFixedBioGeo COMPARE_base_rest (EXPECTED FAILURE)
    FAIL ERS_D_Ld15.f45_f45_mg37.I2000Clm50FatesRs.derecho_gnu.clm-FatesColdTwoStreamNoCompFixedBioGeo BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF
    FAIL PEM_D_Ld15.5x5_amazon.I2000Clm50FatesRs.derecho_gnu.clm-FatesColdSeedDisp COMPARE_base_modpes (EXPECTED FAILURE)
    FAIL SMS_D_Ld3.f09_g17.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhen_prescribed RUN time=94 (EXPECTED FAILURE)
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_gnu.clm-FatesPRISM--clm-NEON-FATES-YELL SHAREDLIB_BUILD time=162 (UNEXPECTED: expected FAIL)
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_gnu.clm-FatesPRISM--clm-NEON-FATES-YELL RUN time=103 (UNEXPECTED: expected FAIL)

 
1210-094439de_int: 38 tests
   --->  FAIL ERP_P128x2_Ld30.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_intel.clm-FatesColdSatPhen BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF
    FAIL ERP_P256x2_Ld30.f45_f45_mg37.I2000Clm60FatesRs.derecho_intel.clm-mimicsFatesCold RUN time=49 (EXPECTED FAILURE)
    PEND ERP_P256x2_Ld30.f45_f45_mg37.I2000Clm60FatesRs.derecho_intel.clm-mimicsFatesCold COMPARE_base_rest
    FAIL ERS_D_Ld15.f45_f45_mg37.I2000Clm50FatesRs.derecho_intel.clm-FatesColdTwoStream COMPARE_base_rest (EXPECTED FAILURE)
    --> PEND ERS_D_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdLandUse SHAREDLIB_BUILD (UNEXPECTED: expected FAIL)
    --> FAIL ERS_D_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdLUH2 BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF
    --> FAIL ERS_D_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdLUH2HarvestArea BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF
    --> FAIL ERS_D_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdLUH2HarvestMass BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF
    --> FAIL ERS_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdNoCompFixedBioGeo BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF
    FAIL ERS_Ld30.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_intel.clm-FatesColdSatPhen BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF
    FAIL ERS_Ld60.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdST3 RUN time=26 (EXPECTED FAILURE)
    PEND PVT_Lm3.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesLUPFT RUN
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_intel.clm-FatesFireLightningPopDens--clm-NEON-FATES-NIWO SHAREDLIB_BUILD time=137 (UNEXPECTED: expected FAIL)
    FAIL SMS_Lm1.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_intel.clm-FatesColdBasic BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF

 
1210-094439de_nvh: 1 test
    FAIL SMS.f45_f45_mg37.I2000Clm60FatesSpRsGs.derecho_nvhpc.clm-FatesColdSatPhen BASELINE fates-sci.1.80.4_api.37.0.0-ctsm5.3.012: DIFF

[rgknox]

I'm realizing there is a subtle flaw in the approach to this PR. When in nocomp and fixed-biogeo modes (including and excluding SP mode), there is an assumption that the patch index is equivalent to the PFT index of that patch. While the most of logic in the changes hold, this discontinuity affects output such as PCT_NAT_VEG, and other variables that have this assumption.
I'm going to circle back and take on a slightly different approach. We will continue to allow a zero index patch, but we will also have a global integer that states if the first index is a zero or a one. This will allow us to get rid of these "if patchno==nocomp_bareground" filters.

[rgknox]

As per the previous comment (Dec 28th 2024), this PR has been re-structured. The idea of shifting indexes and abandoning the zero patch created too many issues. The current set of changes seeks only to make our methods more consistent.

1. Instead of iterating a patch number index while looping patches, we now always use the patch%patchno to get a boundary condition index for our bc_in and bc_out arrays.
2. We no longer filter nocomp patches based on patchno == 0, and instead compared if the patch's nocomp_pft_label equals nocomp_bareground.
3. There was an inconsistency in how we calculated vegetation temperatures. Originally I thought this inconsistency was causing issues related to nocomp bareground patches, where those patches had uninitialized temperatures. After investigation, those patches appear to have vegetation temperatures initialized from air temperature. So these changes are not as much of a bug fix related to the other changes, and may be more appropriate stand-alone as an improvement. See this diff: 36a86e6

[rgknox]

The updated set of changes does generate B4B results with base using the fates test suite, when comparing to a special base that incorporated only item "3" in the previous post.