Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New patch insertion method #1155

Merged
merged 12 commits into from
Oct 28, 2024
Merged

New patch insertion method #1155

merged 12 commits into from
Oct 28, 2024

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Jan 31, 2024

Sort patches in a simple way that orders them by land use, no comp PFT, and age.

Description:

As part of #1116, I realized there is a simpler way to sort the patches that takes into account all of their continuous (age) and categorical (possibly LU and/or PFT) variables, without needing to have a lot of nested logic. This way the patches will be in some predictable order, which may or may not be important, but seems better than not having them in any predictable order. I separated this from the rest of #1116 and submit it here via a git cherry-pick onto main because it should almost certainly introduce non-B4B changes.

The idea is to make a pseudo-age that takes into account the various patch labels, and then sort by that. The pseudo age is to take some large number N that a patch will never be older than (10,000 years, nominally), and make the pseudo-age = LU * N**2 + PFT * N + age.

Collaborators:

Discussed with @glemieux.

Expectation of Answer Changes:

I would expect this to change answers.

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

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
Copy link
Contributor

glemieux commented Feb 5, 2024

@ckoven thanks for isolating this in a new PR. Is this required for #1116 or will that PR run without this update?

@ckoven
Copy link
Contributor Author

ckoven commented Feb 5, 2024

I don't think there are any required dependencies either way.

@glemieux glemieux self-requested a review March 4, 2024 19:05
@glemieux glemieux self-assigned this Jun 3, 2024
@rgknox rgknox self-requested a review August 12, 2024 19:34
@rgknox
Copy link
Contributor

rgknox commented Sep 18, 2024

@ckoven I think this looks solid. We may also want to pre-multiply any constants into compiled constants in the new function. Compilers may already to it, but why not avoid math when possible.

Updates default parameter file and adds improvements to understory survival

This includes parameter file updates to the default grass allometry,
distinguishes between canopy and understory leaf turn over and adds two
new arctic shrub pfts
Copy link
Contributor

@rgknox rgknox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting this to approve, my request to change the indexing math is really unecessary. The pseudo patch ages would seem to hit a maximum of 1e9, which is well within the holding range for an 8-byte real.

@rgknox
Copy link
Contributor

rgknox commented Oct 21, 2024

Plenty of diffs with base generated on this test:

ryan@derecho:/glade/derecho/scratch/rgknox/tests_1018-111221de> ./cs.status.fails 
1018-111221de_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.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL PEM_D_Ld15.f10_f10_mg37.I2000Clm50FatesRs.derecho_gnu.clm-FatesColdSeedDisp COMPARE_base_modpes (EXPECTED FAILURE)
    FAIL SMS_D_Ld3.f09_g17.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhen_prescribed RUN time=184 (EXPECTED FAILURE)
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_gnu.clm-FatesPRISM--clm-NEON-FATES-YELL SHAREDLIB_BUILD time=301 (UNEXPECTED: expected FAIL)
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_gnu.clm-FatesPRISM--clm-NEON-FATES-YELL RUN time=121 (UNEXPECTED: expected FAIL)

 
1018-111221de_int: 38 tests
    FAIL ERP_Ld9.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdAllVars COMPARE_base_rest
    FAIL ERP_P128x2_Ld30.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_intel.clm-FatesColdSatPhen COMPARE_base_rest
    FAIL ERP_P256x2_Ld30.f45_f45_mg37.I2000Clm60FatesRs.derecho_intel.clm-mimicsFatesCold RUN time=105 (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.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL ERS_D_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdLUH2HarvestArea BASELINE fates-sci.1.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL ERS_D_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdLUH2HarvestMass BASELINE fates-sci.1.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL ERS_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdNoCompFixedBioGeo BASELINE fates-sci.1.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL ERS_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdNoComp BASELINE fates-sci.1.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL ERS_Ld30.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_intel.clm-FatesColdSatPhen COMPARE_base_rest
    FAIL ERS_Ld60.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdLogging BASELINE fates-sci.1.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL ERS_Lm12.1x1_brazil.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesFireLightningPopDens COMPARE_base_rest
    FAIL ERS_Lm13.f45_f45_mg37.I2000Clm50Fates.derecho_intel.clm-FatesColdNoComp BASELINE fates-sci.1.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL ERS_P128x1_Lm25.f10_f10_mg37.I2000Clm60Fates.derecho_intel.clm-FatesColdNoComp BASELINE fates-sci.1.79.0_api.36.1.0-ctsm5.3.006: DIFF
    FAIL PVT_Lm3.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesLUPFT BASELINE fates-sci.1.79.0_api.36.1.0-ctsm5.3.006: DIFF
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_intel.clm-FatesFireLightningPopDens--clm-NEON-FATES-NIWO SHAREDLIB_BUILD time=338 (UNEXPECTED: expected FAIL)

@glemieux
Copy link
Contributor

glemieux commented Oct 23, 2024

I decided to investigate the differences that we're seeing with non-landuse test cases and found what I think is ultimately a minor issue, but something that maybe should be addressed. I noticed that the diffs are happening immediately on the second timestep (i.e. the first timestep after initialization). Writing out the variables necessary for calculating the psuedopatch age and picking a random process, I found that the patch order was incorrect based on the calculated psuedo age.

What I think is happening is that the new method flips the assumed order of the nocomp patches on a site via the psuedo patch age calculation. In the new method, higher pft numbers are associated with "older" patches (i.e. pfts 1 -> 14 go younger to older). This is in contrast to the way we initialize the patches which is to iterate starting at pft 1:

fates/main/EDInitMod.F90

Lines 827 to 830 in f5bd625

new_patch_nocomp_loop: do n = 1, num_nocomp_pfts
! set the PFT index for patches if in nocomp mode.
if(hlm_use_nocomp.eq.itrue)then
nocomp_pft = n

and then insert the patches in that order:

fates/main/EDInitMod.F90

Lines 853 to 876 in f5bd625

new_patch_area_gt_zero: if(newparea .gt. min_patch_area_forced) then
allocate(newp)
call newp%Create(age, newparea, i_lu_state, nocomp_pft, &
num_swb, numpft, sites(s)%nlevsoil, hlm_current_tod, &
regeneration_model)
if (is_first_patch) then !is this the first patch?
! set pointers for first patch (or only patch, if nocomp is false)
newp%patchno = 1
newp%younger => null()
newp%older => null()
sites(s)%youngest_patch => newp
sites(s)%oldest_patch => newp
is_first_patch = .false.
else
! Set pointers for N>1 patches. Note this only happens when nocomp mode is on, or land use is on.
! The new patch is the 'youngest' one, arbitrarily.
newp%patchno = nocomp_pft + (i_lu_state-1) * numpft
newp%older => sites(s)%youngest_patch
newp%younger => null()
sites(s)%youngest_patch%younger => newp
sites(s)%youngest_patch => newp
end if

Given that the initial layout is 1 -> 14, oldest to youngest, and given that InsertPatch is only ever called except upon disturbance, this means that it will take some time for the patches to flip to the assumed state in the new method.

As such, the fix for this would be to either flip the initialization scheme or flip the psuedo patch age scheme.

The current scheme assumes that "older" patches are lower in patch
number index and "younger" patches are larger in patch number index.
@glemieux
Copy link
Contributor

glemieux commented Oct 25, 2024

Retesting just the FatesColdNoComp testmod using the fix pushed via ea22249 is still show diffs against baseline, but looking at the additional diagnostics I added it seems like this is mostly just down to roundoff variation in the initial patch fusions. I'm going to rerun the tests against baselines with the last updates.

@glemieux
Copy link
Contributor

Regression testing on derecho completed over the weekend. The results are mostly b4b, with the exception of the landuse and nocomp based tests, as expected. The satellite phenology only differ in the fieldlist per 147d411. @rgknox there is one issue in that the ST3 test fails run as it can't find FATES_ERROR_EL which is include in the test user_nl_clm. I can fix this locally to make sure the test runs sucessfully, but I wanted to double check that we should work to remove this from the testmod in the future.

test location: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1155-restest-1

@glemieux
Copy link
Contributor

Manually editing the ST3 test user namelist to remove FATES_ERROR_EL results in only fieldlist differences for that test. I've made a ctsm issue to make sure we remove this at the next CTSM PR update opportunity: ESCOMP/CTSM#2850

@glemieux glemieux merged commit 4ffab32 into NGEET:main Oct 28, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants