Skip to content

ctsm5.4.050: Remove FatesColdSatPhen testmod #4155

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.

Sign up for GitHub

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

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

ctsm5.4.050: Remove FatesColdSatPhen testmod #4155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
012516d
FatesColdBasic testmod is now FatesColdDailyHist.
samsrabin Aug 6, 2026
d1eaf7a
Delete FatesColdSatPhen testmod.
samsrabin Aug 6, 2026
18f49ff
FATES SatPhen testmods: Explicitly set use_fates_sp to true.
samsrabin Aug 6, 2026
a7e3035
Remove redundant history file settings from 2 testmods.
samsrabin Aug 6, 2026
f924f9b
Add temporary remove-fatescoldsatphen-testmod test category.
samsrabin Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions cime_config/testdefs/testlist_clm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2539,10 +2539,11 @@
<option name="wallclock">00:20:00</option>
</options>
</test>
<test name="SMS" grid="f45_f45_mg37" compset="I2000Clm60FatesSpRsGs" testmods="clm/FatesColdSatPhen">

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do really like this more descriptive name.

<test name="SMS" grid="f45_f45_mg37" compset="I2000Clm60FatesSpRsGs" testmods="clm/FatesColdDailyHist">
<machines>
<machine name="derecho" compiler="nvhpc" category="aux_clm"/>
<machine name="derecho" compiler="nvhpc" category="fates"/>
<machine name="derecho" compiler="nvhpc" category="remove-fatescoldsatphen-testmod"/>

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From your notes you'll remove this new temporary testlist when you are done, which is good.

</machines>
<options>
<option name="wallclock">00:20:00</option>
Expand Down Expand Up @@ -3627,7 +3628,9 @@
<test name="ERI_D_Ld9" grid="f45_f45_mg37" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdSatPhenCamLndTuningMode">
<machines>
<machine name="derecho" compiler="intel" category="aux_clm"/>
<machine name="derecho" compiler="intel" category="remove-fatescoldsatphen-testmod"/>
<machine name="derecho" compiler="gnu" category="fates"/>
<machine name="derecho" compiler="gnu" category="remove-fatescoldsatphen-testmod"/>
</machines>
<options>
<option name="wallclock">00:20:00</option>
Expand Down Expand Up @@ -4130,11 +4133,13 @@
<option name="wallclock">00:40:00</option>
</options>
</test>
<test name="SMS_D" grid="1x1_brazil" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdSatPhen">
<test name="SMS_D" grid="1x1_brazil" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdDailyHist">
<machines>
<machine name="derecho" compiler="intel" category="ctsm_sci"/>
<machine name="derecho" compiler="intel" category="prebeta"/>
<machine name="derecho" compiler="intel" category="remove-fatescoldsatphen-testmod"/>
<machine name="derecho" compiler="gnu" category="fates"/>
<machine name="derecho" compiler="gnu" category="remove-fatescoldsatphen-testmod"/>
</machines>
<options>
<option name="wallclock">00:20:00</option>
Expand Down Expand Up @@ -4164,35 +4169,39 @@
<test name="SMS_D_Ld3" grid="f09_g17" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdSatPhen_prescribed">
<machines>
<machine name="derecho" compiler="gnu" category="fates"/>
<machine name="derecho" compiler="gnu" category="remove-fatescoldsatphen-testmod"/>
</machines>
<options>
<option name="wallclock">00:30:00</option>
<option name="comment">Test with prescribed LAI and soil moisture with FatesSP mode</option>

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could add something here that points out that this should use a FatesSp compset.

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<option name="comment">Test with prescribed LAI and soil moisture with FatesSP mode</option>
<option name="comment">Test with prescribed LAI and soil moisture with FatesSP mode (be sure to use a FatesSp compset along with the FatesColdSatPhen_prescribed testmod)</option>

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might also go other places as well...

@samsrabin samsrabin Aug 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why, though? Is the thinking that these testmods won't work without a FatesSp compset?

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, good question. This is really a testing philosphy type thing for me. Which might need a longer conversation.

But, as one point, I think our testing should be small deviations from standard ways that the model is run. And we should test and retest those standards ways as often as possible. So if you need FatesSp for a test you do it with the FatesSp compset, so that you are testing the standard compset alongside the test that's a deviation from the standard compset.

The other part is to ensure that as things change things don't work in the test -- but would've broken if the FatesSp compset was used. With this the two can evolve seperately, and that could cause problems as those change.

Now, an alturnative way to do this would be to add this to the include_user_mods:

../../../usermods_dirs/clm/fates_sp

as that will ensure they are aligned as the FatesSp compsets are done using user-mods right now. That may change in the future, but adding it explicitly would make it more visible.

Is that sufficient? Or would you like to do a quick chat on this? A longer term discussion on testing philosphy would also be good to do with the group, but that can be later.

@samsrabin samsrabin Aug 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, Erik—your explanation for wanting to always run these tests with a FatesSp compset makes a lot of sense, so I will make a change. (No need for a larger group discussion, as far as I'm concerned.) But since the general motivation behind this PR is "make it hard to do the wrong thing," just adding a note like "make sure to..." doesn't feel sufficient.

The way I see it, there are two options:

  1. Your idea of including ../../../usermods_dirs/clm/fates_sp.
  2. Adding a shell_commands line that fails the run if it doesn't use a FatesSp compset.

Which would you prefer?

@ekluzek ekluzek Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh. You are right it's better to make have a way to ensure it's followed over just a reminder to do it.

I would be good with either. But I'd say I prefer the later with it aborting if you do a mismatch.

</options>
</test>
<test name="SMS_Lm1" grid="f45_f45_mg37" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdBasic">
<test name="SMS_Lm1" grid="f45_f45_mg37" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdDailyHist">
<machines>
<machine name="derecho" compiler="intel" category="fates"/>
<machine name="derecho" compiler="intel" category="remove-fatescoldsatphen-testmod"/>
</machines>
<options>
<option name="wallclock">00:40:00</option>
<option name="comment">Smoke test that uses just the FATES SP compset.</option>
</options>
</test>
<test name="ERS_Ld30" grid="f45_f45_mg37" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdSatPhen">
<test name="ERS_Ld30" grid="f45_f45_mg37" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdDailyHist">
<machines>
<machine name="derecho" compiler="intel" category="fates"/>
<machine name="derecho" compiler="intel" category="remove-fatescoldsatphen-testmod"/>
</machines>
<options>
<option name="wallclock">00:40:00</option>
<option name="comment">30 day exact restart test for FATES fixed biogeography reduced complexity mode on an f45 grid.</option>
</options>
</test>
<test name="ERP_P128x2_Ld30" grid="f45_f45_mg37" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdSatPhen">
<test name="ERP_P128x2_Ld30" grid="f45_f45_mg37" compset="I2000Clm60FatesSpCruRsGs" testmods="clm/FatesColdDailyHist">
<machines>
<machine name="derecho" compiler="intel" category="fates"/>
<machine name="derecho" compiler="intel" category="aux_clm"/>
<machine name="derecho" compiler="intel" category="prebeta"/>
<machine name="derecho" compiler="intel" category="remove-fatescoldsatphen-testmod"/>
</machines>
<options>
<option name="wallclock">00:40:00</option>
Expand Down
0 ...ds_dirs/clm/FatesColdBasic/shell_commands → ...irs/clm/FatesColdDailyHist/shell_commands
View file
Open in desktop
File renamed without changes.
0 ...ods_dirs/clm/FatesColdSatPhen/user_nl_clm → ...s_dirs/clm/FatesColdDailyHist/user_nl_clm
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion cime_config/testdefs/testmods_dirs/clm/FatesColdDryDepSatPhen/include_user_mods
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
../FatesColdBasic
../FatesColdDailyHist
5 changes: 1 addition & 4 deletions cime_config/testdefs/testmods_dirs/clm/FatesColdDryDepSatPhen/user_nl_clm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1 @@

hist_mfilt = 1
hist_nhtfrq = -24

use_fates_sp = .true.

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I really think that we shouldn't set this in the testmod, that we should use the FatesSp compset. I think there are other things besides use_fates_sp that are set there now, and even if there aren't they may well be more in the future.

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now, your notes give this:

"The justification: If you're only testing that standard FATES SP mode works, use a FatesSp compset so that you also test the compset. That's why we deleted the FatesColdSatPhen testmod—to ensure the FatesSp compsets are tested. However, these other testmods exercise special SP-mode settings. They should still exist, and they should all set FATES SP mode as a backup in case a non-FatesSp compset is chosen."

OK, I can see that.

But, add a comment that says that in the user_nl_clm. I think the tests for these modes should use the FatesSp compset, but as they are generally testing something slightly outside of that, it's probably OK to have that here.

2 changes: 1 addition & 1 deletion cime_config/testdefs/testmods_dirs/clm/FatesColdMeganSatPhen/include_user_mods
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
../FatesColdBasic
../FatesColdDailyHist
5 changes: 1 addition & 4 deletions cime_config/testdefs/testmods_dirs/clm/FatesColdMeganSatPhen/user_nl_clm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1 @@

hist_mfilt = 1
hist_nhtfrq = -24

use_fates_sp = .true.

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above add a comment that talks about this and that in general this should be used with a FatesSP compset.

2 changes: 2 additions & 0 deletions cime_config/testdefs/testmods_dirs/clm/FatesColdSatPhen
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
To test standard FATES SP mode configurations, use a FatesSp compset instead of a testmod.

1 change: 0 additions & 1 deletion cime_config/testdefs/testmods_dirs/clm/FatesColdSatPhen/include_user_mods
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion cime_config/testdefs/testmods_dirs/clm/FatesColdSatPhenCamLndTuningMode/include_user_mods
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
../FatesColdSatPhen
../FatesColdDailyHist
2 changes: 1 addition & 1 deletion cime_config/testdefs/testmods_dirs/clm/FatesColdSatPhen_prescribed/include_user_mods
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
../FatesColdSatPhen
../FatesColdDailyHist
1 change: 1 addition & 0 deletions cime_config/testdefs/testmods_dirs/clm/FatesColdSatPhen_prescribed/user_nl_clm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use_fates_sp = .true.

@ekluzek ekluzek Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above add a comment about this.

use_lai_streams = .true.
lai_tintalgo = 'lower' ! set time interpolation to use lower value, so can compare more directly to input dataset