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

Add FATES land use change module to pass LUH2 data to FATES #2076

Merged
merged 39 commits into from
Dec 31, 2023

Conversation

glemieux
Copy link
Collaborator

@glemieux glemieux commented Jul 27, 2023

Description of changes

This pull request adds a new module, the structure of which is adopted from the dyn_subgrid code, to enable hlms to ingest raw luh2 data to be passed to FATES. A new namelist variable is introduced use_fates_luh, to engage the module functionality, which is off by default. The luh2 dataset to be ingested is a minimally processed concatenation of the luh2 state, transition and management datasets.

The luh2 dataset is generated by the fates data tool found in NGEET/fates#1032. This pull request is associated with NGEET/fates#1040.

Specific notes

Contributors other than yourself, if any: @ckoven

CTSM Issues Fixed (include github issue #):
Fixes #1077

Are answers expected to change (and if so in what way)? No change expected for non-luh2 run modes

Any User Interface Changes (namelist or namelist defaults changes)? Yes, this adds a new set of namelist variables for the luh2 data file and

Testing performed, if any: Will do standard

FATES API update to facilitate fates refactor

This updates a number of FATES type names and module use statements
which correspond with a refactoring effort that moves FATES
patches and cohorts into their own respective modules.

With the FATES update is a minor science update, so there are
changes to answers for FATES.

This also incorporates a minor update to a more recent version
of the ccs config external.
Rename hist fields to track them down more easily

Renaming history fields to make easier to find in lists, e.g. when
using ncview. For example, litter fields like MET_LIT and STR_LIT
will be LIT_MET and LIT_STR.
@glemieux glemieux marked this pull request as ready for review August 22, 2023 17:42
@glemieux glemieux mentioned this pull request Aug 25, 2023
5 tasks
Refactor some max_patch_per_col and maxsoil_patches loops

As explained in issue 2025, the old loop structure was out-of-date and
needed a refactor, partly because use of max_patch_per_col was limited,
partly to simplify the code and make it easier to read and understand,
and partly to improve (even if slightly) model performance.
@ekluzek ekluzek added enhancement new capability or improved behavior of existing capability FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM tag: enh - new science labels Oct 16, 2023
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

@glemieux this is an exciting upgrade for CTSM/FATES so it's exciting it's starting to come in.

I have a few requests that I think should be straight forward and easy. Let me know if you want to meet to discuss.

I have one other question. How will the workflow to create these files work? I think we will add that to the CTSM5.2 Makefile right? And it will exercise FATES python scripts to make them right? That's something that we should discuss a bit...

bld/CLMBuildNamelist.pm Outdated Show resolved Hide resolved
bld/namelist_files/namelist_definition_ctsm.xml Outdated Show resolved Hide resolved
bld/CLMBuildNamelist.pm Show resolved Hide resolved
bld/namelist_files/namelist_definition_ctsm.xml Outdated Show resolved Hide resolved
src/dyn_subgrid/dynFATESLandUseChangeMod.F90 Show resolved Hide resolved
src/dyn_subgrid/dynFATESLandUseChangeMod.F90 Outdated Show resolved Hide resolved
src/dyn_subgrid/dynFATESLandUseChangeMod.F90 Outdated Show resolved Hide resolved
src/dyn_subgrid/dynFATESLandUseChangeMod.F90 Show resolved Hide resolved
src/main/controlMod.F90 Outdated Show resolved Hide resolved
src/main/controlMod.F90 Outdated Show resolved Hide resolved
@ekluzek
Copy link
Collaborator

ekluzek commented Oct 16, 2023

@ckoven sorry I didn't get this until now. We had this slated as coming in after cross-seed dispersal as it was looking more complete. We might need to think about how to relay general FATES priorities to CTSM to make sure we are in sync. Something we are trying to do in CTSM now is bring things in that are ready to go. A goal with that is to bring things in ASAP when possible, and hopefully will get us bringing in PR's quicker than we have been. But, at the same time some work has a higher priority than something that is already ready to go. So we are going to need to balance between those at times.

@ckoven
Copy link
Contributor

ckoven commented Oct 16, 2023

Thanks @ekluzek and no worries!

@wwieder
Copy link
Contributor

wwieder commented Nov 3, 2023

Excited for this to come in. Thanks for working on it @glemieux and @ckoven .

After this is merged, it seems like a logical next step to try and transient, historical no-comp baseline? I wondered if it makes sense for @samsrabin to do this run? We'll also likely need to start building up capacity to post-process results. These seem like high FATES priorities for the LMWG and I'm wondering how best to make them a reality?

@ckoven
Copy link
Contributor

ckoven commented Nov 3, 2023

Thanks @wwieder — in principle I totally agree; in practice I think it might make sense to wait until the second set of land-use-change mods are ready, which I am still working on. Basically the set of changes in this PR provides the infrastructure for land-use change, but won't yet really be scientifically meaningful for a few reasons. The main one of which is that there is in this PR no enforcement of land cover properties on patches with a given land use. I.e. there is nothing that makes pasture lands or croplands behave differently from secondary lands here. In the short term, the way to accomplish this is (1) the linkage of the land-use classes with the nocomp logic, to enforce a different PFT composition on patches with different land use classes, and (2) starting to apply land management to the different land use classes, e.g. grazing on pasture and rangelands. In the long term, number 2 above can be expanded while number 1 can be relaxed, so that land cover follows more fully as a result of management rather than direct imposition of land cover (except in cases like crops where imposition of land cover is a direct land management practice). So once this first land-use PR is in, I have subsequent branches that will start to address both of those points, as well as other things needed to do transient runs such as the capability to spin up the model under potential vegetation, and then transition from that to historical land use. A less-fundamental but still relevant point is that we are working towards initial calibrated version of FATES in a nocomp configuration, but not yet a full-FATES version; so we don't have a parameter set to run through a global full-FATES with land-use transient run, but we are much closer to having an initial nocomp global calibration. So as part of this second set of land-use modifications, I do intend to do some global transient historical runs in nocomp configuration.

Merging 2156 2148 2233 2235 2237 2044

For details please see the individual PRs and/or the ChangeLog.

Multiple contributors, as listed in the individual PRs and the ChangeLog.

Answers change for 3 tests:
ERS_Lm20_Mmpi-serial.1x1_smallvilleIA.I2000Clm50BgcCropQianRs.izumi_gnu.clm-cropMonthlyNoinitial BASELINE ctsm5.1.dev151: DIFF
SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm51Bgc.izumi_nag.clm-default--clm-NEON-NIWO BASELINE ctsm5.1.dev151: DIFF
SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm51Bgc.izumi_nag.clm-NEON-MOAB--clm-PRISM BASELINE ctsm5.1.dev151: DIFF
- The first is due to the bug fix in 2237; the diffs are in the fill
patterns of 3 fields.
- The second and third are due to the bug fix in 2044.
Use baset_latvary parameters

Namelist parameters baset_latvary_slope and baset_latvary_intercept were never actually used, with values of 0.4 and 12 being hard-coded in the relevant subroutine instead. This PR fixes that, and also adds unit testing of a refactored function that uses them.
@glemieux
Copy link
Collaborator Author

I should have noted NGEET/fates#1040 (comment) here instead; after discussing the carbon balance checking with @rgknox, we've realized that the wood product should not be included in the check for fates runs. As a result this has been removed via 1a1ec94.

Various BFB fixes and updates

Purpose/description of changes
------------------------------
the default comes in a later tag (slevis)

Regular and python testing passed.

Does not change answers relative to dev158.
@glemieux
Copy link
Collaborator Author

@ekluzek Generating fates baselines prior to testing this PR branch with ctsm5.1.dev159 and comparing against the previous fates baseline based on ctsm5.1.dev158 is B4B with one exception: the FatesCold 13 month 1x1_brazil smoke test is showing diffs. What's interesting is the gnu version of this test is B4B against the previous tag baseline.

Also note that for aux_clm, while we have testmods that run over a year, we only have reduced complexity tests, which might be the reason the previous tag was coming back B4B. Maybe we should migrate this particular single site smoke test to aux_clm as well?

@glemieux
Copy link
Collaborator Author

glemieux commented Dec 14, 2023

@ekluzek Generating fates baselines prior to testing this PR branch with ctsm5.1.dev159 and comparing against the previous fates baseline based on ctsm5.1.dev158 is B4B with one exception: the FatesCold 13 month 1x1_brazil smoke test is showing diffs. What's interesting is the gnu version of this test is B4B against the previous tag baseline.

@rgknox noted that the issue is that that one test for the previous fates baseline was rebuilt with the wrong fates version so that DIFF can be ignored.

@ekluzek
Copy link
Collaborator

ekluzek commented Dec 14, 2023

Ahh, good I'm glad @rgknox figured it out. I'll redo that baseline test. Sorry about that, that's my bad.

@glemieux
Copy link
Collaborator Author

glemieux commented Dec 15, 2023

I'm seeing failing RUNs with fates NEON testmods on izumi. The issue appears to be that the weight has the wrong dimensions (one beyond what is expected):

Runtime Error: /home/glemieux/ctsm/src/main/surfrdUtilsMod.F90, line 238: Invalid reference to procedure SURFRDUTILSMOD:COLLAPSE_TO_DOMINANT - Dummy array WEIGHT (number 1) has 18 elements but actual argument only has 17 elements
Program terminated by fatal error
/home/glemieux/ctsm/src/main/surfrdUtilsMod.F90, line 238: Error occurred in SURFRDUTILSMOD:COLLAPSE_TO_DOMINANT
/home/glemieux/ctsm/src/main/surfrdMod.F90, line 854: Called by SURFRDMOD:SURFRD_VEG_ALL
/home/glemieux/ctsm/src/main/surfrdMod.F90, line 192: Called by SURFRDMOD:SURFRD_GET_DATA
/home/glemieux/ctsm/src/main/clm_initializeMod.F90, line 247: Called by CLM_INITIALIZEMOD:INITIALIZE2
/home/glemieux/ctsm/src/cpl/nuopc/lnd_comp_nuopc.F90, line 659: Called by LND_COMP_NUOPC:INITIALIZEREALIZE
/home/glemieux/ctsm/components/cmeps/cime_config/../cesm/driver/esmApp.F90, line 128: Called by ESMAPP

I'm testing non-fates NEON sites right now via the izumi aux_clm run, but looking at recent commits, I'm kind of at a loss for what could have caused these failures. Note that all other fates izumi testmods are passing and the gnu compiler versions of these NEON tests passed on derecho.

@glemieux
Copy link
Collaborator Author

glemieux commented Dec 15, 2023

I'm testing non-fates NEON sites right now via the izumi aux_clm run, but looking at recent commits, I'm kind of at a loss for what could have caused these failures. Note that all other fates izumi testmods are passing and the gnu compiler versions of these NEON tests passed on derecho.

I think I know what this is: in NGEET/fates#1040, the number of maximum fates patches per site went up by one (to 17) which sets the natpft_ub to 17 for fates. The NEON surface dataset is read for setting the surfpft_ub, which is then used to allocate the pft bounds for wt_nat_patch; the data set has 16 pfts. The issue crops up in that the collapse_to_dominant call passes in wt_nat_patch and natpft_ub to set the input argument bounds and then finds the discrepency.

The simplest fix to align these is to update the new fates parameter file coming in with this PR to reduce the number of maximum patches back to 16. @ckoven does dropping the max number of patches for the primary landunit by one (to 9) sound appropriate?

As an aside, @ekluzek should the collapse_to_dominant call actually be passing the surfpft bounds as input since that's how it's being allocated?

I'm a little surprised that this passed on derecho. My guess is that since the dummy argument definition inside collapse_to_dominant was getting set with a larger number than what was coming in with the actual called argument allocation, the compilers there didn't complain.

@ekluzek
Copy link
Collaborator

ekluzek commented Dec 15, 2023

@glemieux offhand that does sound right to me. Do you want to look at the code together? Or do you want to put together the code changes you think should happen and have us look it over?

@glemieux
Copy link
Collaborator Author

glemieux commented Dec 15, 2023

@ekluzek the code change would be pretty straightforward, but it'll be easier to walk through it together given the code leading up to that point. The thing that I want to test out first is what the failure mode would look like when this fix is in. Let me give that a run and I'll tag up with you via chat later.

@ckoven
Copy link
Contributor

ckoven commented Dec 15, 2023

@ckoven does dropping the max number of patches for the primary landunit by one (to 9) sound appropriate

@glemieux yes that makes sense to me. thanks!

A newer parameter file was generated that reduces the maximum
number of fates patches for the default configuration.  This
avoids misalignment between the default surface datasets.
The wt_nat_patch array is allocated using surfpft bounds.  While
surfpft and natpft will typically match, this isn't the case
for fates currently.  As such the nag compiler will complain
if the input array has lower bounds for its dimensions than
the dummy argument bounds.
@glemieux
Copy link
Collaborator Author

Status update: aux_clm and fates restesting on izumi has been kicked off and is in the queue. I'll test aux_clm (and retest fates) on derecho once izumi testing comes back clean.

@glemieux
Copy link
Collaborator Author

Regression testing the aux_clm suite on izumi is complete. All expected tests passed B4B with the expected exception of the fates testmods. This is expected as the reorganization of the disturbance code will result in small differences showing up in the longer test (like the one Ld30 fates testmod). All other fates diffs are FIELDLIST differences due to known history output name changes.

Izumi folder location: /home/glemieux/scratch/ctsm-tests/tests_pr2076_aux-clm

@glemieux
Copy link
Collaborator Author

aux_clm testing on derecho is complete. All expected tests pass B4B with the known exception of the fates test mods, which is due to known science updates since the API 31.0 update (including the landuse PR).

derecho location: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr2079_aux_clm

A quick note that I needed to rebuild the DAE test to get it to pass RUN even though the logs suggest everything ran fine and the COMPARE_base_rest passed. The Teststatus.log had the following which was tripping up the test reporter immediately after the COMPARE_base_rest output:

 620  ---------------------------------------------------
 621 2023-12-18 17:25:19: ERROR: ERROR: Unrecognized line ('/bin/bash: module: line 1: syntax error: unexpected end of file
 622 ') found in /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr2079_aux_clm/DAE_C2_D_Lh12.f10_f10_mg37.I2000Clm50BgcCrop.derecho_intel.clm-DA_multidrv.GC.pr2079_aux_clm_int/run/case2run     /da.log.2668444.desched1.231218-172242.gz
 623  ---------------------------------------------------

Hat tip to @slevis-lmwg for pointing me to the fact that he was seeing the same thing as note with #2253 (comment) and that a resubmitted fixed his case.

Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

I worked on a couple issues here, filing a couple issues and resolving the conversations that were addressed.

src/main/controlMod.F90 Show resolved Hide resolved
src/dyn_subgrid/dynFATESLandUseChangeMod.F90 Outdated Show resolved Hide resolved
bld/CLMBuildNamelist.pm Show resolved Hide resolved
src/dyn_subgrid/dynFATESLandUseChangeMod.F90 Outdated Show resolved Hide resolved
bld/CLMBuildNamelist.pm Show resolved Hide resolved
@ekluzek ekluzek merged commit adbc727 into ESCOMP:master Dec 31, 2023
2 checks passed
@ekluzek ekluzek deleted the fates-luh2 branch December 31, 2023 06:45
@samsrabin samsrabin added the science Enhancement to or bug impacting science label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM science Enhancement to or bug impacting science
Projects
Status: Done (non release/external)
5 participants