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

Fix issue#1163 #1165

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Fix issue#1163 #1165

merged 1 commit into from
Sep 30, 2020

Conversation

olyson
Copy link
Contributor

@olyson olyson commented Sep 29, 2020

Fix issue #1163 (onset_counter/offset_counter variables on restart files that were based on 30-minute time step simulation is not compatible with subsequent model runs using a 20 minute time step)

Specific notes

Contributors other than yourself, if any: billsacks

CTSM Issues Fixed (include github issue #): 1163.

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

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any:
The following tests were bfb with ctsm5.1.dev003:
ERP_D_Ld5.f10_f10_musgs.IHistClm50BgcCrop.cheyenne_intel.clm-allActive
ERS_D_Ld6.f10_f10_musgs.I1850Clm45BgcCrop.cheyenne_intel.clm-clm50CMIP6frc
ERS_Ly5_P144x1.f10_f10_musgs.IHistClm51BgcCropGs.cheyenne_intel.clm-cropMonthOutput

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

Thanks @olyson !

@billsacks
Copy link
Member

billsacks commented Sep 30, 2020

aux_clm tests pass; everything is bit-for-bit except one test: SMS_Ln9.ne30pg2_ne30pg2_mg17.I2000Clm50BgcCrop.cheyenne_intel.clm-clm50cam6LndTuningMode. This test uses an initial conditions file that was created from a run with a 450 sec time step (/glade/p/cesmdata/cseg/inputdata/lnd/clm2/initdata_map/clmi.F2000.2000-01-01.ne120pg3_mt13_simyr2000_c200728.nc), but the run itself uses a 1800 sec time step. So I believe that these answer changes are expected, and that the earlier runs of this test were buggy.

Thinking about that test made me want to think a bit more about the implications of these changes for various changes in time step. I'll post my thoughts here soon. I don't expect to come up with a reason not to merge this PR, but I want to give it a bit more thought before merging.

@billsacks
Copy link
Member

Okay, as I note in #1167 , I don't think the changes here are completely robust with respect to what's intended in CNPhenology - particularly related to what seems to be an intention that certain code gets executed exactly once, in the second to last time step of the onset / offset periods.

However, my sense is that the changes in this PR are still an improvement over what was in place before. So, since I've already done testing on this PR, my plan is to bring this PR to master as is. Then we can address #1167 separately, or possibly decide that it isn't worth addressing.

@billsacks billsacks merged commit 08e5a90 into ESCOMP:master Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (non release/external)
Development

Successfully merging this pull request may close these issues.

2 participants