Open billsacks opened 3 years ago
For one, organic_frac_squared should not have changed from CLM5 to CLM5.1.
On Tue, Sep 29, 2020 at 8:41 PM Bill Sacks notifications@github.com wrote:
Brief summary of bug
I am reviewing the changes in ctsm5.0.dev001. I noticed a few possible problems with the namelist defaults in that tag. I'm not sure if all of these are actually bugs, but I wanted to bring this to the attention of @ekluzek https://github.com/ekluzek and others in case some should be fixed. One of these issues, organic_frac_squared, appears to lead to incorrect results in out-of-the-box clm51 configurations (unless this change was intentional). The other issues would just affect runs in non-standard configurations, or are cleanup issues. General bug information
CTSM version you are using: ctsm5.1.dev001
Does this bug cause significantly incorrect results in the model's science? Yes, I think so, for clm51 cases, because of incorrect value for organic_frac_squared.
Configurations affected: clm51 cases, and non-standard configurations Details of bug
These are really a few somewhat unrelated issues. I'm grouping them together here for ease of review:
- organic_frac_squared for clm5_1 uses the same setting as clm4_5; I'm guessing this is wrong:
- Some fire-related namelist variables depend on the physics version directly, whereas my intuition is that they should depend on the fire method. As a thought experiment: if someone sets up a clm51 case but reverts to the li2016crufrc fire method, or conversely sets up a clm50 case but changes to li2021gswpfrc, what defaults should be used for these? Specifically, this is the case for the lightning and popdens streams: the light_res variable and the three popdens variables that depend on phys version – though it looks like the popdens variables don't actually differ with different phys versions, so the duplication in xml is irrelevant and could probably be removed:
- It looks like some fire-related namelist variables would be undefined for certain values of lnd_tuning_mode, when using li2016crufrc: rh_low and pot_hmn_ign_counts_alpha. I think these should be given default values for li2016crufrc without a lnd_tuning_mode attribute, so that they are given some reasonable value for other lnd_tuning_modes (I think that's the right way to fix this problem). This could arise in practice if someone sets up a clm51 case but reverts to the li2016crufrc fire method, or conversely sets up a clm50 case but changes to li2021gswpfrc (and probably other ways, too):
- Cleanup that shouldn't have any impact: spinup_state has some duplicated lines for fates: there are 2 unique lines here, each doubled:
-
Cleanup that shouldn't have any impact: the 2010 control use case has some unnecessary clm4_0 settings
Cleanup that shouldn't have any impact: the 2010 control use case has a duplicated line:
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CTSM/issues/1166, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFABYVCZ6K76S7F62WKPU23SIKLHDANCNFSM4R6Q6SJA .
I was thinking a bit about how you could test to ensure that there are no other unintentional changes between clm50 and clm51. @ekluzek maybe you've already done something like this, but if not, I'll share my thoughts:
The simplest thing to do, which would pick up problems with the default configuration but not with modified configurations, would be to do one multi-year run with a clm51 case and its clm50 equivalent; in the clm51 case, revert the few things that have changed intentionally (tuning mode, fire method, param file, lightning stream info; maybe others?) back to their clm50 values. I think these should be bit-for-bit.
I think that more rigorous testing could be done as follows:
OK, I looked into the history of the organic_frac_squared setting to figure out where this bug happened. It can be instructive to look into it in order to see how to prevent such things in the future. I thought this likely started with the original branch based off of release-clm5.0, but it was correct there. The error came in when I used patch to merge in to the latest version. I don't think it was a problem with patch itself, but in my wrong interpretation of it. But, possibly this puts another tick in favor of re-base over patch.
In most cases the build-namelist unit testing would also catch this problem, but because the baseline didn't have clm5_1 to compare to -- it couldn't. A test I didn't do that might have been useful was to compare it to the release-clm5.0 version. The problem is all the other namelist differences would swamp out the results. I did that now, and I agree with that assessment. Technically the problem shows up, but I likely wouldn't have seen it. But, a simple test I could have done was to compare the clm5_0 namelist with clm5_1, when I do that the problem pops out. It also shows other fire namelist settings as correct, and two fire namelist settings that depend on the lnd_tuning_mode settings. But, I could then validate those as correct. So that would've been a better way to do this.
The other thing I thought of doing but didn't was to run a simulation in the release version, and then one in the updated version. But, because answers are expected to be different and the namelists are expected to be different I still don't think that would've been useful.
@ekluzek If you haven't already, I still feel it could be good to do some testing like what is discussed in https://github.com/ESCOMP/CTSM/issues/1166#issuecomment-701507777 to ensure that there aren't other issues like this. Although, as you point out, it's probably sufficient to compare the namelists... but ideally this namelist comparison would be done across a range of configurations, not just a single test.
So far I've compared namelists, and I'll show the list below. The one additional problem I've identified is the finidat setting for 2000 for clm5_1 physics is wrong. the list of tests includes various resolutions, bgc settings, crop on and off, as well as spinup. The bad finidat for 2000 won't matter for simulations where you startup from a REFCASE you define. I'll add this to the list above.
Here's the list of namelist comparisons...
./lnd_in.clm5_1-bgc.-res+1.9x2.5+-bgc+bgc+-crop+-use_case+1850_control+-envxml_dir+.+-namelist+++a+start_ymd=18500101++ ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-light_res+360x720 ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-envxml_dir+.++-bgc+sp+-envxml_dir+.+-vichydro ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-envxml_dir+.++-bgc+bgc+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+0.9x1.25+-bgc+bgc+-crop+-use_case+1850_control+-envxml_dir+.+-namelist+++a+start_ymd=18500101++ ./lnd_in.clm5_1-bgc.-bgc+bgc+-crop+-res+ne30np4+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+5x5_amazon+-use_case+2000_control ./lnd_in.-phys+clm5_1.-res+5x5_amazon+ ./lnd_in.-phys+clm5_1.-res+10x15+-envxml_dir+.++-bgc+bgc+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-envxml_dir+.++-bgc+sp+-envxml_dir+.+-vichydro ./lnd_in.-phys+clm5_1.-res+0.125nldas2+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+ne30np4.pg2+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+1.9x2.5+-bgc+bgc+-crop+-use_case+1850-2100_SSP3-7.0_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+1x1_brazil+-namelist+++a+use_lch4=.true.,use_nitrif_denitrif=.true.++ ./lnd_in.-phys+clm5_1.-res+10x15+-envxml_dir+.++-bgc+bgc+-dynamic_vegetation+-ignore_warnings ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-namelist+++a+use_c13=.true.,use_c14=.true.,use_c14_bombspike=.true.++ ./lnd_in.-phys+clm5_1.-res+ne120np4.pg3+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+10x15+-envxml_dir+.++-bgc+bgc+-clm_demand+flanduse_timeseries+-sim_year+1850-2000+-namelist+++a+start_ymd=18500101++ ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-mask+gx1v6+-sim_year+1850+-envxml_dir+.+-lnd_tuning_mod+clm5_1_GSWP3v1+-bgc+bgc ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-clm_accelerated_spinup=on ./lnd_in.clm5_1-bgc.-bgc+bgc+-crop+-res+4x5+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+C96+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+10x15+-bgc+bgc+-crop+-use_case+1850-2100_SSP3-7.0_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+5x5_amazon+-clm_accelerated_spinup+on ./lnd_in.clm5_1-bgc.-res+10x15+-envxml_dir+.+-bgc+bgc ./lnd_in.-phys+clm5_1.-res+4x5+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+1.9x2.5+-bgc+bgc+-crop+-use_case+1850-2100_SSP2-4.5_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.clm5_1-bgc.-res+10x15+-bgc+bgc+-crop+-use_case+1850_control+-envxml_dir+.+-namelist+++a+start_ymd=18500101++ ./lnd_in.-phys+clm5_1.-res+64x128+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+C24+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+1.9x2.5+ ./lnd_in.-phys+clm5_1.-res+10x15+ ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-envxml_dir+.++-bgc+bgc+-dynamic_vegetation+-ignore_warnings ./lnd_in.clm5_1-bgc.-res+0.9x1.25+-use_case+20thC_transient+-envxml_dir+.+-namelist+++a+start_ymd=18500101++ ./lnd_in.clm5_1-bgc.-res+4x5+-envxml_dir+.+-bgc+bgc ./lnd_in.-phys+clm5_1.-res+C192+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+0.9x1.25+-bgc+bgc+-crop+-use_case+1850-2100_SSP1-2.6_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+10x15+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-light_res+360x720 ./lnd_in.-phys+clm5_1.-res+10x15+-namelist+++a+use_lch4=.true.,use_nitrif_denitrif=.true.++ ./lnd_in.-phys+clm5_1.-res+ne16np4+-envxml_dir+.++-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-bgc+bgc+-crop+-res+1.9x2.5+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+10x15+-clm_accelerated_spinup+on ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-envxml_dir+.++-bgc+bgc+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+10x15+-use_case+20thC_transient+-envxml_dir+.+-namelist+++a+start_ymd=18500101++ ./lnd_in.clm5_1-bgc.-bgc+bgc+-crop+-res+1x1_smallvilleIA+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+ne30np4+-envxml_dir+.+-bgc+bgc ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-namelist+++a+use_lch4=.true.,use_nitrif_denitrif=.true.++ ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-use_case+2000_control ./lnd_in.-phys+clm5_1.-res+5x5_amazon+-namelist+++a+use_lch4=.true.,use_nitrif_denitrif=.true.++ ./lnd_in.-phys+clm5_1.-res+C384+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+48x96+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+ne30np4+-use_case+20thC_transient+-envxml_dir+.+-namelist+++a+start_ymd=18500101++ ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-clm_accelerated_spinup=on ./lnd_in.-phys+clm5_1.-res+1x1_brazil+-use_case+2000_control ./lnd_in.clm5_1-bgc.-res+1.9x2.5+-bgc+bgc+-crop+-use_case+1850-2100_SSP5-8.5_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.clm5_1-bgc.-res+ne16np4+-envxml_dir+.+-bgc+bgc ./lnd_in.-phys+clm5_1.-res+10x15+-use_case+2000_control ./lnd_in.clm5_1-bgc.-res+1.9x2.5+-use_case+20thC_transient+-envxml_dir+.+-namelist+++a+start_ymd=18500101++ ./lnd_in.-phys+clm5_1.-res+10x15+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-namelist+++a+use_c13=.true.,use_c14=.true.,use_c14_bombspike=.true.++ ./lnd_in.-phys+clm5_1.-res+ne16np4+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+10x15+-bgc+bgc+-crop+-use_case+1850-2100_SSP1-2.6_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+ne16np4+-envxml_dir+.++-bgc+bgc+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+1.9x2.5+-bgc+bgc+-crop+-use_case+1850-2100_SSP1-2.6_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.clm5_1-bgc.-bgc+bgc+-crop+-res+10x15+-envxml_dir+. ./lnd_in.clm5_1-bgc.-bgc+bgc+-crop+-res+1x1_numaIA+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+ne0np4.ARCTIC.ne30x4+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-envxml_dir+.++-bgc+bgc+-clm_demand+flanduse_timeseries+-sim_year+1850-2000+-namelist+++a+start_ymd=18500101++ ./lnd_in.clm5_1-bgc.-bgc+bgc+-crop+-res+0.9x1.25+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+10x15+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-clm_accelerated_spinup=on ./lnd_in.-phys+clm5_1.-res+ne30np4+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+10x15+-envxml_dir+.++-bgc+sp+-envxml_dir+.+-vichydro ./lnd_in.-phys+clm5_1.-res+ne0np4.ARCTICGRIS.ne30x8+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+ne30np4.pg3+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+0.9x1.25+-bgc+bgc+-crop+-use_case+1850-2100_SSP3-7.0_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+1x1_brazil+-clm_accelerated_spinup+on ./lnd_in.-phys+clm5_1.-res+10x15+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+0.9x1.25+-bgc+bgc+-crop+-use_case+1850-2100_SSP2-4.5_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+1x1_brazil+ ./lnd_in.clm5_1-bgc.-res+10x15+-bgc+bgc+-crop+-use_case+1850-2100_SSP5-8.5_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+C48+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-namelist+++a+irrigate=.true.+++-crop+-bgc+cn++-envxml_dir+.+-bgc+cn+-crop ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-clm_accelerated_spinup+on ./lnd_in.clm5_1-bgc.-res+1.9x2.5+-envxml_dir+.+-bgc+bgc ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-envxml_dir+.++-bgc+bgc+-dynamic_vegetation+-ignore_warnings ./lnd_in.-phys+clm5_1.-res+1x1_urbanc_alpha+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-mask+gx1v6+-sim_year+1850+-envxml_dir+.+-lnd_tuning_mod+clm5_1_GSWP3v1+-bgc+sp ./lnd_in.clm5_1-bgc.-res+0.9x1.25+-bgc+bgc+-crop+-use_case+1850-2100_SSP5-8.5_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-bgc+sp+-envxml_dir+. ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-light_res+360x720 ./lnd_in.-phys+clm5_1.-res+ne0np4CONUS.ne30x8+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+10x15+-bgc+bgc+-crop+-use_case+1850-2100_SSP2-4.5_transient+-envxml_dir+.+-namelist+++a+start_ymd=20150101++ ./lnd_in.-phys+clm5_1.-res+1.9x2.5+-envxml_dir+.++-bgc+bgc+-clm_demand+flanduse_timeseries+-sim_year+1850-2000+-namelist+++a+start_ymd=18500101++ ./lnd_in.-phys+clm5_1.-res+0.9x1.25+-envxml_dir+.++-bgc+bgc+-envxml_dir+.+-namelist+++a+use_c13=.true.,use_c14=.true.,use_c14_bombspike=.true.++ ./lnd_in.-phys+clm5_1.-res+ne120np4.pg2+-bgc+sp+-envxml_dir+. ./lnd_in.clm5_1-bgc.-res+0.9x1.25+-envxml_dir+.+-bgc+bgc
Thanks @ekluzek . I think the extensive namelist comparisons are sufficient: no need to do actual test runs as I originally suggested.
Brief summary of bug
I am reviewing the changes in ctsm5.0.dev001. I noticed a few possible problems with the namelist defaults in that tag. I'm not sure if all of these are actually bugs, but I wanted to bring this to the attention of @ekluzek and others in case some should be fixed. One of these issues, organic_frac_squared, appears to lead to incorrect results in out-of-the-box clm51 configurations (unless this change was intentional). The other issues would just affect runs in non-standard configurations, or are cleanup issues.
General bug information
CTSM version you are using: ctsm5.1.dev001
Does this bug cause significantly incorrect results in the model's science? Yes, I think so, for clm51 cases, because of incorrect value for organic_frac_squared.
Configurations affected: clm51 cases, and non-standard configurations
Details of bug
These are really a few somewhat unrelated issues. I'm grouping them together here for ease of review:
organic_frac_squared
forclm5_1
uses the same setting asclm4_5
; I'm guessing this is wrong:https://github.com/ESCOMP/CTSM/blob/08bc0ade130b23a67a9366120f8033897505c74b/bld/namelist_files/namelist_defaults_ctsm.xml#L163-L165
[x] Default finidat files for sim-year 2000 are incorrect (fixed in ctsm5.1.dev008)
[ ] Some fire-related namelist variables depend on the physics version directly, whereas my intuition is that they should depend on the fire method. As a thought experiment: if someone sets up a clm51 case but reverts to the li2016crufrc fire method, or conversely sets up a clm50 case but changes to li2021gswpfrc, what defaults should be used for these? Specifically, this is the case for the lightning and popdens streams: the
light_res
variable and the threepopdens
variables that depend on phys version – though it looks like thepopdens
variables don't actually differ with different phys versions, so the duplication in xml is irrelevant and could probably be removed:https://github.com/ESCOMP/CTSM/blob/08bc0ade130b23a67a9366120f8033897505c74b/bld/namelist_files/namelist_defaults_ctsm.xml#L1569-L1573
https://github.com/ESCOMP/CTSM/blob/08bc0ade130b23a67a9366120f8033897505c74b/bld/namelist_files/namelist_defaults_ctsm.xml#L1592-L1602
rh_low
andpot_hmn_ign_counts_alpha
. I think these should be given default values for li2016crufrc without alnd_tuning_mode
attribute, so that they are given some reasonable value for other lnd_tuning_modes (I think that's the right way to fix this problem). This could arise in practice if someone sets up a clm51 case but reverts to the li2016crufrc fire method, or conversely sets up a clm50 case but changes to li2021gswpfrc (and probably other ways, too):https://github.com/ESCOMP/CTSM/blob/08bc0ade130b23a67a9366120f8033897505c74b/bld/namelist_files/namelist_defaults_ctsm.xml#L265-L270
https://github.com/ESCOMP/CTSM/blob/08bc0ade130b23a67a9366120f8033897505c74b/bld/namelist_files/namelist_defaults_ctsm.xml#L276-L281
https://github.com/ESCOMP/CTSM/blob/08bc0ade130b23a67a9366120f8033897505c74b/bld/namelist_files/namelist_defaults_ctsm.xml#L53-L56
[ ] Cleanup that shouldn't have any impact: the 2010 control use case has some unnecessary
clm4_0
settings[ ] Cleanup that shouldn't have any impact: the 2010 control use case has a duplicated line:
https://github.com/ESCOMP/CTSM/blob/08bc0ade130b23a67a9366120f8033897505c74b/bld/namelist_files/use_cases/2010_control.xml#L11-L12