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

New params file with changes for Meier roughness, MIMICS, and SNICAR, and changes to leafcn and k*_nonmyc #2258

Merged
merged 18 commits into from
Nov 22, 2023

Conversation

slevis-lmwg
Copy link
Contributor

@slevis-lmwg slevis-lmwg commented Nov 15, 2023

Description of changes

  1. Start using existing new params file for Meier roughness:
    /glade/campaign/cesm/cesmdata/inputdata/lnd/clm2/paramdata/ctsm51_params.RMz0.c231011.nc
    and include bug-fix Meier roughness changes answers on processor count #2219
  2. Update forcing heights per Forcing heights need to be updated when roughness length/displacement height is updated #2071.
  3. Update params file for MIMICS per MIMICS related parameters missing name and units in paramfile #1845.
  4. Make leafcn for pfts 15 and 16 the same per leafcn for pfts 15 and 16 have diverged, should probably be the same #2184.
  5. Switch the values of params kc_nonmyc and kn_nonmyc per Switched parameters for non-mycorrhizal N uptake costs in FUN #2120.
  6. Move SNICAR parameters to params file per Move SNICAR parameter to parameter file #2247.

Specific notes

Contributors other than yourself, if any:
@ekluzek @olyson @wwieder @ecaas @cenlinhe others?

CTSM Issues Fixed (include github issue #):
Fixes #2219
Fixes #2071
Fixes #1845
Fixes #2184
Fixes #2120
Fixes #2247

Are answers expected to change (and if so in what way)? YES.

  • In step (1) answers change due to the new values for zlnd and zsno in the RMz0 params file.
  • In step (2) answers will change due to a change in how we calculate forcing heights in the default ZengWang2007 method.
  • In step (3) answers change within roundoff for MIMICS cases due to changed order of operations.
  • In step (4) answers will change because we will return leafcn for pft 15 to the same value as pft 16, i.e. 25.
  • In step (5) answers change because we switch two parameters back to the way that they should have been.
  • In step (6) answers may not change, unless due to a change in order of operations.

Any User Interface Changes (namelist or namelist defaults changes)?
New params file.

Testing performed, if any:
The answer-changes discussion above pretty much covers the current extent of testing.
After I merge this PR, I will run an Answer-changing simulation to compare with the model before this merge.

@slevis-lmwg slevis-lmwg self-assigned this Nov 15, 2023
@slevis-lmwg slevis-lmwg added priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations PR status: work in progress next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Nov 15, 2023
@slevis-lmwg slevis-lmwg requested a review from olyson November 15, 2023 23:51
@slevis-lmwg
Copy link
Contributor Author

@olyson I'm requesting a review from you because you wrote #2071. Could you check my work in this PR as pertains to #2071?

@slevis-lmwg
Copy link
Contributor Author

slevis-lmwg commented Nov 16, 2023

I have pushed steps (1) and (2) to the PR.
Locally in my branch I am up to step (5). The cheyenne test-suite returns no fails when issued this command:
./cs.status.fails | grep -v '152: DIFF' | grep -v 'EXPECTED FAILURE' | grep -v NLCOMP

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we bother lining up "observational" with the rest of the variable descriptions (off by one space).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this is the vertical alignment question. Which I am confused about our current status on. We used to make sure things aligned vertically. But, recently I had understood we were going to go away from that. But, then we have indications that we aren't. So we need to nail this down and all agree on what we are doing.

Copy link
Contributor

@olyson olyson left a comment

Choose a reason for hiding this comment

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

One comment, otherwise looks good, thanks.

@slevis-lmwg
Copy link
Contributor Author

slevis-lmwg commented Nov 18, 2023

Test-suites:

  • izumi WAITING for now
  • cheyenne RERUN A FEW TESTS making new baselines for them:
    Meier2022_surf_rough
    ciso_cwd_hr
    OR if changing default z0param_method in this PR, then rerun all tests, making new baselines for all.

@slevis-lmwg
Copy link
Contributor Author

@wwieder @ekluzek @olyson:
Is this the PR when we also change z0param_method from ZengWang2007 to Meier2022?
This far I have not done that.

@wwieder
Copy link
Contributor

wwieder commented Nov 18, 2023

Yes, I think so, Sam

@slevis-lmwg
Copy link
Contributor Author

slevis-lmwg commented Nov 18, 2023

I looked through the Meier tests in the cheyenne test-suite and found no exact Clm51 equivalents, so I will keep them without the reference to Meier2022_surf_rough, because the latter is now default for clm51.

One test failed on izumi:
SMS_Ld5_Mmpi-serial.1x1_brazil.IHistClm50BgcQianRs.izumi_gnu.clm-mimics
It passed when I changed it to
SMS_Ld5_Mmpi-serial.1x1_brazil.IHistClm51Bgc.izumi_gnu.clm-mimics
I confirmed with Will that it's fine to assume that MIMICS works with ctsm51 and not clm45 or clm50.
I generated a baseline for this new test.

Four tests failed on cheyenne:

ERP_D_Ld10_P36x2.f10_f10_mg37.IHistClm51BgcCrop.cheyenne_intel.clm-ciso_decStart
ERS_Ly5_P144x1.f10_f10_mg37.IHistClm51BgcCrop.cheyenne_intel.clm-cropMonthOutput
LCISO_Lm13.f10_f10_mg37.IHistClm51BgcCrop.cheyenne_intel.clm-ciso_monthly
SMS_D_Ln6.f09_g17.I1850Clm50BgcNoAnthro.cheyenne_intel.clm-Meier2022_surf_rough

The first three I fixed with with a code change that permits htop < 1e-10 for istcrop.
I generated baselines for these three.

The fourth should have been a clm51 test, but I get
ERROR: Invalid compset name, I1850Clm51BgcNoAnthro so I will delete this test because it is redundant as a clm50 test.

This expected failure should be a clm51 test, so I will rerun and generate a baseline.
ERP_P72x2_Ld30.f45_f45_mg37.I2000Clm50FatesRs.cheyenne_intel.clm-mimicsFatesCold
This test was failing and still fails with
Fire emission can NOT be on when FATES is also on.
This could be fixed by pointing this testmod to /basic instead of /default; however, the change would affect all mimics tests, so we're in a bind at the moment.

I'm rerunning the four clm51 tests that had Meier testmods, now without those testmods.
I'm generating baselines for these four.

I hope I got everything...

@ekluzek
Copy link
Collaborator

ekluzek commented Nov 18, 2023

Yes, this is correct the original card in upcoming tags had this in it:

The commit in question is:

a75a4ba

To use the commit you need to use cherry-pick so it only brings in that one change, rather than all the changes on CESM3_dev. Or you can copy the changes in by hand. cherry-pick keeps the history, that's it's advantage...

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 have a question and then an important change to make. Since, it's so simple I'm approving it as it is so you can immediately move forward after the one change.

Nice, to see this coming in.

@@ -1,4 +1,2 @@
z0param_method = 'Meier2022'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should remove this test mod since the clm4_5 and clm5_0 tests already are doing Meier off, and clm5_1 with Meier on. So both options are covered. So there's no reason for a test mod that does one or the other...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ekluzek
Copy link
Collaborator

ekluzek commented Nov 20, 2023

@slevis-lmwg since you've run testing and the changes are still relatively modest I think we should go forward with this. But, it does look like there are a mix of changes that could be done as bit-for-bit and others that truly are answer changing. It's good to make the answer changes as small as possible, and do the bit-for-bit parts separate, just in case there's a mistake in the bit-for-bit part. As then it'll show up as an unexpected answer change. That kind of thing is hidden when there's a mix of both.

For example, the move of fresh_snw_rds_max to the param file could be done as a bit-for-bit part. If it changes answers you then know there's a mistake in there. But, it's hidden when it comes in with answer changes.

It does look like you marked clm4_5 and clm5_0 as changing answers. Is that expected and is it possible to make them identical to dev153? If they can be made identical that would ensure that the bit-for-bit parts are bit-for-bit as expected.

@@ -17,15 +17,6 @@
<option name="wallclock">00:20:00</option>
</options>
</test>
<test name="SMS_D_Ln6" grid="f09_g17" compset="I1850Clm50BgcNoAnthro" testmods="clm/Meier2022_surf_rough">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the Meier testmods made this test redundant as a clm50 test, so I removed it.
(I did first try changing to a clm51 test and got Invalid compset name, I1850Clm51BgcNoAnthro.)

@slevis-lmwg
Copy link
Contributor Author

Due to updating some tests and then generating baselines for them after running the test-suites, I am rerunning the test-suites and comparing against ctsm5.1.dev154.

@ekluzek when I'm satisfied with the results, I will look into the feasibility of your suggestion of separating the parts of this PR that should be bfb.

@slevis-lmwg
Copy link
Contributor Author

slevis-lmwg commented Nov 21, 2023

@slevis-lmwg since you've run testing and the changes are still relatively modest I think we should go forward with this. But, it does look like there are a mix of changes that could be done as bit-for-bit and others that truly are answer changing. It's good to make the answer changes as small as possible, and do the bit-for-bit parts separate, just in case there's a mistake in the bit-for-bit part. As then it'll show up as an unexpected answer change. That kind of thing is hidden when there's a mix of both.

For example, the move of fresh_snw_rds_max to the param file could be done as a bit-for-bit part. If it changes answers you then know there's a mistake in there. But, it's hidden when it comes in with answer changes.

It does look like you marked clm4_5 and clm5_0 as changing answers. Is that expected and is it possible to make them identical to dev153? If they can be made identical that would ensure that the bit-for-bit parts are bit-for-bit as expected.

Here are my thoughts in the same order that this PR's commits went in:
f9978db and b8c71fa: These two change answers for Meier2022 (clm51) but the latter commit also changes zlnd and zsno, which changes all three CLMs.
626f520: Takes out if (z0param_method == 'Meier2022'), so changes answers for all three CLMs
319d194: Changes answers for mimics and, therefore clm51 only (order of ops, so roundoff)
2ee6943: Changes clm51 params file, so affects clm51 only (expect more than roundoff)
f185a31: bfb
6dc1966: This git merge escomp/master probably does change answers from previous commit
29ca5ad and 71e174f: Puts snicar params on the params files for all three CLMs; I don't know whether we'd expect bfb or roundoff...

@ekluzek I see in your post that you're suggesting letting this through without additional testing at this point. I do think that additional testing would be convoluted and time consuming but doable. How about I wait for your confirmation (I could also meet if you prefer) by beginning of tomorrow, before moving ahead with the tag?

@slevis-lmwg
Copy link
Contributor Author

slevis-lmwg commented Nov 22, 2023

From the above, I started a relatively easy test on izumi:
I backed up to 6dc1966 (before the snicar mods), then I put back the changes of the commit right after snicar (71e174f). Comparing to dev154 (the new tag's baseline that I've created), I hope to find at most roundoff diffs.

Update: The above passed bfb against dev154 on izumi (12 gnu, 18 intel, and 32 nag tests).
Other changes are quite confined, so I will not pursue further testing, other than the Answer Changing Tags simulation after making the tag tomorrow.

New clm5_1 params file with new parameters and with modified values of existing parameters.
New clm5_0 and clm4_5 params files with new parameters for SNICAR.
./rimport on the new params files fails with "No space left on device" but the 4 files are safe here:
/glade/u/home/slevis/paramfiles/*_params.c231117.nc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Final comment before I merge:
./rimport on the new params files fails with "No space left on device"
The files are safe in my home directory: /glade/u/home/slevis/paramfiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update I have now completed this pending item:

./rimport -file lnd/clm2/paramdata/clm45_params.c231117.nc
./rimport -file lnd/clm2/paramdata/clm50_params.c231117.nc
./rimport -file lnd/clm2/paramdata/ctsm51_params.c231117.nc
./rimport -file lnd/clm2/paramdata/ctsm51_ciso_cwd_hr_params.c231117.nc

@slevis-lmwg slevis-lmwg marked this pull request as ready for review November 22, 2023 19:50
@slevis-lmwg slevis-lmwg merged commit bfbd39f into ESCOMP:master Nov 22, 2023
@slevis-lmwg slevis-lmwg deleted the new_params_file branch November 22, 2023 19:51
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Nov 27, 2023
New params files for Meier roughness, MIMICS, SNICAR, and with changes to leafcn and k*_nonmyc

1) Start using existing new params file for Meier roughness:
/glade/campaign/cesm/cesmdata/inputdata/lnd/clm2/paramdata/ctsm51_params.RMz0.c231011.nc
and include bug-fix ESCOMP#2219
2) Update forcing heights per ESCOMP#2071.
3) Update params file for MIMICS per ESCOMP#1845.
4) Make leafcn for pfts 15 and 16 the same per ESCOMP#2184.
5) Switch the values of params kc_nonmyc and kn_nonmyc per ESCOMP#2120.
6) Move SNICAR parameters to params file per ESCOMP#2247.

Changes answers. Details in PR ESCOMP#2258 and in the ChangeLog.
@samsrabin samsrabin removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Mar 21, 2024
@samsrabin samsrabin added bug something is working incorrectly science Enhancement to or bug impacting science labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is working incorrectly priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations science Enhancement to or bug impacting science
Projects
Status: Done (non release/external)
5 participants