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

FATES land use v2 API update (CTSM-side) #2507

Open
wants to merge 122 commits into
base: master
Choose a base branch
from

Conversation

samsrabin
Copy link
Contributor

@samsrabin samsrabin commented May 1, 2024

Description of changes

Update CTSM to work with FATES land use v2 (NGEET/fates#1116) .

Based on @glemieux's analogous update for E3SM (E3SM-Project/E3SM#6353).

Specific notes

This PR enables the host land model to read in a new landuse x pft static mapping dataset from the fates landuse data tool. A default output at a 4x5 resolution is provided.

This update also includes a new ctsm-fates specific system test using the PVT prefix which provides for a 5 year spin-up in the new fates "potentival vegetation" mode the output of which is then used to start a fates landuse transient run using the landuse timeseries data (which was added back with ctsm5.1.dev160).

The fates harvest and logging options have been refactored and simplified into a new option, fates_harvest_mode, to aid the user in selecting harvest modes compatible with other fates run modes. This includes two new modes that use the area or mass harvesting data from the fates LUH2 landuse timeseries data. A new convenience namelist option, use_fates_lupft has also been provided for turning on fates landuse with no competition and fixed biogeography.

Contributors other than yourself, if any: @glemieux, @ckoven

CTSM Issues Fixed (include github issue #): None

Are answers expected to change (and if so in what way)? Yes, but for fates testmods only

Any User Interface Changes (namelist or namelist defaults changes)? Yes.

  • New use_fates_lupft convenience option (use_fates_luh + use_fates_nocomp + use_fates_fixedbiogeog)
  • use_fates_logging refactored into fates_harvest_mode with the addition of two new harvest modes

Testing performed, if any: In progress with development.

@samsrabin samsrabin added FATES update requiring an API change Changes to the FATES version that also REQUIRE an API change in CTSM PR status: work in progress PR: author feels this is NOT ready to merge to master labels May 1, 2024
@samsrabin samsrabin self-assigned this May 1, 2024
@wwieder wwieder added this to the 2024 CESM June workshop milestone May 2, 2024
@glemieux
Copy link
Contributor

glemieux commented Jul 9, 2024

This pull request is now updated with the fates-side tag for land use v2. Regression testing against ctsm5.2.008 is showing mostly B4B results with expected DIFFs for fates testmods. There are two unexpected ctsm failures that I'm seeing:

FAIL LILACSMOKE_D_Ld2.f10_f10_mg37.I2000Ctsm50NwpSpAsRs.derecho_intel.clm-lilac MODEL_BUILD time=329
FAIL RXCROPMATURITYSKIPGEN_Ld1097.f10_f10_mg37.IHistClm50BgcCrop.derecho_intel.clm-cropMonthOutput RUN time=24

The RXCROP run failure looks like its just a variation on #2322, i.e. the string element for fsurdat is too long for the gddgen case. When I rerun the regression tests for the final integration testing, I'll make sure not to use a specific test-id, assuming the default system test-id is short enough.

The LILACSMOKE build failure is not clear to me yet.

Copy link
Contributor

@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.

This is really great to see come in. Seeing this helps me to understand how all of this works. I think the instructions are getting more clear. And I like the array of possible options for fates_harvest_mode. I like the error checking being done and new tests. The code is using parameters to save integer settings needed which is great.

I'm marking as approved even though I ask for a few changes just to facilitate moving forward. There are a couple longer term things that I'll make into issues. I also setup a meeting to go over this with @glemieux to talk about some of this.

event_code: fates logging via fates logging event codes (see fates parameter file) only
surfdata_file: fates harvest driven by CLM landuse timeseries data (dynHarvestMod)
luhdata_area: fates harvest driven by LUH2 raw harvest data, area-based (dynFATESLandUseChangeMod)
luhdata_mass: fates harvest driven by LUH2 raw harvest data, mass-based (dynFATESLandUseChangeMod)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the line. Deprecated is misspelled here, so that would be good to fix. I also wonder about adding asterisks after landuse_timeseries and before the note so it's obvious that both lines reference the landuse_timeseries option. Is event_code likely to be deprecated in the future as well?

event_code: fates logging via fates logging event codes (see fates parameter file) only
surfdata_file: fates harvest driven by CLM landuse timeseries data (dynHarvestMod)
luhdata_area: fates harvest driven by LUH2 raw harvest data, area-based (dynFATESLandUseChangeMod)
luhdata_mass: fates harvest driven by LUH2 raw harvest data, mass-based (dynFATESLandUseChangeMod)
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple further suggestions. Deprecated is misspelled in the update. Good to fix that.

It also seems to only apply to landuse_timeseries, and maybe event_code needs a similar comment?

I also wondered about adding astericks to the end of landuse_timeseries and then at the begining of the note regarding it -- so it's more obvious they are connected.

cime_config/SystemTests/pvt.py Outdated Show resolved Hide resolved
cime_config/testdefs/testlist_clm.xml Show resolved Hide resolved
src/dyn_subgrid/dynFATESLandUseChangeMod.F90 Show resolved Hide resolved
src/main/controlMod.F90 Show resolved Hide resolved
Pre-generating files is a more likely next step relative to auto-gen
@ekluzek
Copy link
Contributor

ekluzek commented Jul 12, 2024

@samsrabin @glemieux have been going over this extensively. Greg is going to do the testing for this. And then he'll pass it off for someone else to complete the tag. Since, @samrabin you are the author (and we wanted to cycle through FATES tags) we figure you should be the one to make the actual tag. This is just the final steps of the tagging process.

So start at step 16 from:

https://github.com/ESCOMP/CTSM/wiki/Protocols-on-updating-FATES-within-CTSM#fates-updates-that-include-api-changes

When I've done this myself I've typically do some double checking of the work. So I have done up to all of the following steps

  1. Review the final CTSM changes again myself to note for any glaring problems that should kick it back for more testing
  2. Make sure .gitmodules is pointing to a NGEET FATES tag and not a personal branch/hash
  3. Make sure the testing baselines were run and have standard names and are on: izumi, and Derecho for both aux_clm and fates tests
  4. Double check test results are as expected (looking at the their test cases, found either by them giving the directory names in the PR or by looking at the tail of CaseDocs/lnd_in for the directory name in the a test for the baseline)
  5. Double check that the fates version used for the baselines is the same as in .gitmodules for the PR
  6. Review the ChangeLog (maybe do simple updates for clarity or have author update if really needed)
  7. Update the date in the ChangeLog if a day or more has passed (commit and push it to the PR branch)

The double checking has the intent of doing quick checking to make sure everything is good and we won't have problems later. I trust everyone on the project, but I also appreciate having my own work double checked to help prevent problems that become more involved to track down. FATES tags are also more involved than regular tags and having one person do the FATES tag/testing and another finalize the CTSM side has been a good workflow for us.

Pinging @adrifoster as she'll be doing these final steps as well. I'm adding the above steps to a CTSM SE discussion so we can settle on what we all think should be required and what can be optional (I'm thinking require up to step 3, with the later ones optional).

@glemieux
Copy link
Contributor

@samsrabin @ekluzek aux_clm testing against ctsm5.2.011 on derecho is underway.

I'm going to see if I can get things going on izumi.

@glemieux
Copy link
Contributor

glemieux commented Jul 13, 2024

@ekluzek @samsrabin Regression testing on derecho is complete and shows B4B results against ctsm5.2.011 for all non-fates tests with one exception. The RXCROPMATURITYSKIPGEN_Ld1097.f10_f10_mg37.IHistClm50BgcCrop.derecho_intel.clm-cropMonthOutput test failed RUN again with the error that I saw last time:

161 2024-07-12 18:47:39: ERROR: Command /glade/u/home/glemieux/ctsm/bld/build-namelist failed rc=255
162 out=
163 err=ERROR : CLM build-namelist::CLMBuildNamelist::process_namelist_commandline_infile() : Invalid namelist variable in '-infile' /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr2507-aux_clm-final/RXCROPMATURITYSKIPGEN_Ld1097.f10_f10_mg37.IHistClm50BgcCrop.derecho_intel.clm-cropMonthOutput.GC.pr2507-aux_clm-final_int.gddgen/Buildconf/clmconf/namelist.
164  ERROR: in validate_variable_value (package Build::Namelist): Variable name fsurdat has a string element that is too long: '/glade/u/home/glemieux/scratch/ctsm-tests/tests_pr2507-aux_clm-final/RXCROPMATURITYSKIPGEN_Ld1097.f10_f10_mg37.IHistClm50BgcCrop.derecho_intel.clm-cropMonthOutput.GC.pr2507-aux_clm-final_int.gddgen/surfdata_10x15_hist_1850_78pfts_c240216.all_crops_everywhere.nc'

That said, I didn't see an issue for this. Is it a known issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FATES update requiring an API change Changes to the FATES version that also REQUIRE an API change in CTSM PR status: work in progress PR: author feels this is NOT ready to merge to master
Projects
Upcoming tags
In progress - master/b4b-dev
Status: Stalled
Development

Successfully merging this pull request may close these issues.

None yet

5 participants