Make sure at least one test running FatesSp just uses the compset and not a test-mod directory

[adrifoster]

Brief summary of bug

As of the latest FATES API update I believe we now have a mis-match between some of the history exclude variable names set up in the FATES-SP compset (I2000Clm51FatesSpRsGs) and the FATES code.

General bug information

CTSM version you are using: ctsm5.1.dev105

Does this bug cause significantly incorrect results in the model's science? No

Configurations affected: I2000Clm51FatesSpRsGs

Details of bug

We just need to update the variable names in the compset user_nl_clm.

However, it is unfortunate that this bug was not caught in testing. We should probably also make sure that tests that use a compset with included/excluded variables do not get overwritten by those set up in the testing script itself.

Important details of your setup / configuration so we can reproduce the bug

This came up when a new user was following our recent CTSM Global FATES Case tutorial

./create_newcase --case ~/clm_tutorial_cases/I2000_CTSM_FATESsp --res f45_g37 --compset I2000Clm51FatesSpRsGs --run-unsupported

Important output or errors that show the problem

Error message in the lnd log:
`
htapes_fieldlist ERROR:

FATES_GPP_UNDERSTORY in fexcl(

74 ) for history tape 1 not found

ENDRUN:

ERROR: ERROR in histFileMod.F90 at line 852

`

@ekluzek I am happy to tackle at least part of this, if you could point me to where this compset is set up.

[ekluzek]

@adrifoster the user-mod is in cime_config/usermods_dirs/fates_sp. There is a user_nl_clm that needs to be modified. And we'll need to change the test list so that some of the FatesSp tests don't use a test-mod.

[adrifoster]

Okay in looking at this file I am actually not seeing any problems with the history variable names.. So now I'm not sure why the user was seeing that bug. I will go through the tutorial now and see what the deal may be.

[adrifoster]

I believe the user was using an out-dated version of CTSM that was incompatible with their FATES version, so I believe this is actually not a bug.

However, we may still want to make sure some FATES tests use this compset.

[adrifoster]

It seems from investigation of the different testdefs that for some sort of FatesColdDefBase, in shell_commands we need:

./xmlchange CLM_FORCE_COLDSTART="on"
./xmlchange BFBFLAG="TRUE"

In the Fates testdef we also have a user_nl_clm file, but I'm not sure we want to use all of these in a FATES SP test:

hist_mfilt        = 365
hist_nhtfrq       = -24
hist_empty_htapes = .true.
fates_spitfire_mode = 1

fates_spitfire_mode is set to 0 in the fates_sp user mod, and I don't think we want to empty the htapes. I think we can keep the hist_mfilt and hist_nhtfrq?

@ekluzek does this seem correct? If so, I was going to create a new testdefs folder with just a shell_commands file with the xmlchange commands above.

Then in the FatesColdDefReducedComplexSatPhen test def we would just have that new test def in the include_user_mods file as well as the fates_sp user mod. Would we want to add a user_nl_clm file with the hist_mfilt and hist_nhtfrq parameters? The user_nl_ files get appended rather than overwritten?

[ekluzek]

Yes, I think the user-mod should JUST have the shell_commands parts and not the user_nl_clm parts. The user_nl_clm parts do get appended on, rather than overwritten, but that means the order matters. For this test mod we should avoid doing anything to the user_nl_clm, just to make sure it doesn't conflict with the user-mod. So that means you probably need the test with this test mod to run a month so that you'll get output. The question then is if we should have more than one test mod for FatesSP,? One would be this basic one that only does shell_commands, and another one could mess with history settings. Look at the other basic test cases and see what they do about history settings, that could help inform here.

[adrifoster]

@ekluzek @billsacks trying something new here by creating a Design Doc, please let me know what you think!

Software Design Documentation

FATES Test Def Update

Introduction

---

We want to update the FATES test definitions to:

1. at least one test running FATES Satellite Phenology (SP) mode uses just the compset definition and not a test mod directory
2. the test mod directories related to FATES SP mode are robust to future changes to code and other testing requirements, and do not depend on the order in which include_user_mods or the compset parameters are set

Potential Solutions

---

1. keep the current include_user_mods structure, ensuring that the order in which user mods are called is correct
   • Pros: low redundancy, keeps similar structure to other FATES test defs
   • Cons: because the order matters so much, the tests may be prone to user error if anything is changed
2. only use FatesColdDefBasic as an includes, and add requisite files (e.g. shell_commands, user_nl_clm) for each test case inside its own testmod directory
   • Pros: low fragility because each test is mostly self-contained and not reliant on other test defs or order
   • Cons: some redundancy because we use the same files inside multiple test def folders
     • Note that in some cases redudancy/code duplication is actually favored for unit tests and the like, because this ensures stability (see quote below from a software engineering best practices book)

Quote from "Software Engineering at Google" - https://abseil.io/resources/swe-book

One final aspect of writing clear tests and avoiding brittleness has to do with code sharing. Most software attempts to achieve a principle called DRY—"Don’t Repeat Yourself." DRY states that software is easier to maintain if every concept is canonically represented in one place and code duplication is kept to a minimum. This approach is especially valuable in making changes easier because an engineer needs to update only one piece of code rather than tracking down multiple references. The downside to such consolidation is that it can make code unclear, requiring readers to follow chains of references to understand what the code is doing.

In normal production code, that downside is usually a small price to pay for making code easier to change and work with. But this cost/benefit analysis plays out a little differently in the context of test code. Good tests are designed to be stable, and in fact you usually want them to break when the system being tested changes. So DRY doesn’t have quite as much benefit when it comes to test code. At the same time, the costs of complexity are greater for tests: production code has the benefit of a test suite to ensure that it keeps working as it becomes complex, whereas tests must stand by themselves, risking bugs if they aren’t self-evidently correct. As mentioned earlier, something has gone wrong if tests start becoming complex enough that it feels like they need their own tests to ensure that they’re working properly.

Instead of being completely DRY, test code should often strive to be DAMP—that is, to promote "Descriptive And Meaningful Phrases." A little bit of duplication is OK in tests so long as that duplication makes the test simpler and clearer.

---

Design Considerations

---

General Constraints:

Because we are using the FATES and FATES SP compset for the FATES SP tests, some parameters (e.g., those related to spitfire, megan, etc.) as well as some history variable specifications are set up before the files in a --user-mods-dirs directory are applied. This means that order is more tricky with the FATES SP tests, in addition to the requirement of the order being important in the include_user_mods file.

Design and Architecture

---

My preference is to go with Potential Solution 2 above, at least for the test defs related to FATES SP. This removes the issue of order and makes the test more 'DAMP'.

Code design

1. set include_user_mods for FatesColdDefReducedComplexSatPhen, FatesColdDefMeganReducedComplexSatPhen, and FatesColdDefDryDepReducedComplexSatPhen to only the new FatesColdDefBasic test mod directory, which just sets ./xmlchange CLM_FORCE_COLDSTART="on" and ./xmlchange BFBFLAG="TRUE"
2. create a new user_nl_clm file for each of the three test directories above to set hist_mfilt = 365 and hist_nhtfrq = -24. These are the only parameters that would be needed from the Fates test directory - all other parameters set in that directory are incorrect for a FATES SP compset
3. copy the requisite shell_commands files from the FatesColdDefMegan and FatesColdDefDryDep into the FatesColdDefMeganReducedComplexSatPhen and FatesColdDefDryDepReducedComplexSatPhen, respectively. This ensures that these test directories do not need to be added to the include_user_mods file. While this less DRY, it is more DAMP.

Rollout Plan

---

I plan to test each modified test def individually to ensure the parameters are set accurately, as well as run the usual aux_clm and fates test suites.