In transient pft simulations with use_crop=.false., %crop does not advance from 1850 values

[slevis-lmwg]

Brief summary of bug

In traditional transient pft simulations with use_crop = .false., %crop does not advance from 1850 values.

General bug information

CTSM version you are using:
ctsm1.0.dev012-4-ga344044

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

Configurations affected:

• IHist with create_crop_landunit = .true. and use_crop = .false. (which requires do_transient_crops = .false.)
• All transient pft configurations with create_crop_landunit = .true. and use_crop = .false.

Details of bug

Relevant code:
in subgridMod.F90, function crop_patch_exists

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

Test that revealed the problem when I ran it twice:

1. in /glade/work/slevis/git/ctsm1.0.dev012/cime/scripts (baseline master)
2. in /glade/work/slevis/git/collapse_pfts/cime/scripts (branch collapse2gencrop: Collapse2gencrop branch: Allow the model to not need to read 16-pft datasets and rather read 78-pft datasets #483)

ERP_D.f10_f10_musgs.IHistClm50Bgc.cheyenne_gnu.clm-decStart

In the test's 2002 history files I found %crop from 2002 in the branch and from 1850 in the baseline.

[billsacks]

@dlawrenncar @olyson - Sam discovered this and has been in communication with @ekluzek and myself. I want to let you know about it in case it affects any recent simulations.

[wwieder]

@danicalombardozzi has some no crop runs where this may be and issue? How can we tell?

…

On Thu, Oct 18, 2018 at 3:45 PM Bill Sacks ***@***.***> wrote: @dlawrenncar <https://github.com/dlawrenncar> @olyson <https://github.com/olyson> - Sam discovered this and has been in communication with @ekluzek <https://github.com/ekluzek> and myself. I want to let you know about it in case it affects any recent simulations. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#538 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHqLJBZN8WF1gycB1NWajzOMMZI-bCCBks5umPZfgaJpZM4XvJNc> .

-- Will Wieder Project Scientist CGD, NCAR 303-497-1352

[billsacks]

@wwieder @danicalombardozzi - @slevisconsulting can probably answer better than me, but I think you can do a check similar to what he did:

In the test's 2002 history files I found %crop from 2002 in the branch and from 1850 in the baseline.

[wwieder]

We looked at PCT_LANDUNIT, which does change when crops are active, but it didn't change in the 'noCrop' simulation. I think Danica is going to rerun these simulations.

…

On Fri, Oct 19, 2018 at 1:51 PM Bill Sacks ***@***.***> wrote: @wwieder <https://github.com/wwieder> @danicalombardozzi <https://github.com/danicalombardozzi> - @slevisconsulting <https://github.com/slevisconsulting> can probably answer better than me, but I think you can do a check similar to what he did: In the test's 2002 history files I found %crop from 2002 in the branch and from 1850 in the baseline. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#538 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHqLJOWTelleCzoxvF8kgGcirVUt3KQ5ks5umi1UgaJpZM4XvJNc> .

-- Will Wieder Project Scientist CGD, NCAR 303-497-1352

[danicalombardozzi]

Yes, I confirmed that I need to rerun my simulations. Because I am comparing two simulations (one with active crops, one without), I am trying to rerun the simulation using the same version of the code (r267) that I previously used. I've added Sam's code changes and everything compiles, but I get an error message upon job submission that there is no default value found for the flanduse_timeseries. I double-checked that the lnd_in file does specify this file and it exists ( /glade/p/cesmdata/cseg/inputdata/lnd/clm2/surfdata_map/ landuse.timeseries_0.9x1.25_hist_16pfts_Irrig_CMIP6_simyr1850-2015_c170824.nc), but seems not to work in this context. Do we need to use a new land use timeseries file with these changes for the model to run? Danica On Fri, Oct 19, 2018 at 2:11 PM will wieder <notifications@github.com> wrote:

…

We looked at PCT_LANDUNIT, which does change when crops are active, but it didn't change in the 'noCrop' simulation. I think Danica is going to rerun these simulations. On Fri, Oct 19, 2018 at 1:51 PM Bill Sacks ***@***.***> wrote: > @wwieder <https://github.com/wwieder> @danicalombardozzi > <https://github.com/danicalombardozzi> - @slevisconsulting > <https://github.com/slevisconsulting> can probably answer better than me, > but I think you can do a check similar to what he did: > > In the test's 2002 history files I found %crop from 2002 in the branch and > from 1850 in the baseline. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#538 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AHqLJOWTelleCzoxvF8kgGcirVUt3KQ5ks5umi1UgaJpZM4XvJNc > > . > -- Will Wieder Project Scientist CGD, NCAR 303-497-1352 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#538 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AY9tQVm9LIyW1dTfbVAOKU9W7MxuWM9gks5umjIPgaJpZM4XvJNc> .

-- Dr. Danica Lombardozzi she/her/hers Terrestrial Sciences Section Climate and Global Dynamics National Center for Atmospheric Research Boulder, CO 80305 email: dll@ucar.edu office: (303) 497-1777

[billsacks]

I have been looking through the ChangeLog and diffs to try to determine how long this has been broken. My best guess is that this has been broken since clm4_5_15_r236 (May 1, 2017): that's when CLM switched to having create_crop_landunit almost always true.

Another slightly relevant tag seems to be clm4_5_14_r228 (March, 2017), which had a change in initGridCellsMod.F90 that started to pave the way for the changes in r236. But, based on my understanding of the problem and the changes in that tag, this problem wouldn't have shown up for out-of-the-box namelist settings until r236. However, the problem may have shown up earlier than r236 for cases using custom namelist settings for create_crop_landunit along with non-default surface datasets / flanduse_timeseries files.

[ekluzek]

I also asked @wwieder @danicalombardozzi and @olyson if there would ever be a reason you'd want this behavior on purpose (so transient natural PFT's, but fixed crops), and they all agreed (separately) that they couldn't see a reason for that. So I'll put into place a mechanism that will make sure the two are linked and die if they aren't. There's always ways to get around these mechanisms, but it will prevent you from accidentally setting it up in a screwy way.

[billsacks]

@ekluzek - Then should we simply collapse do_transient_crops and do_transient_pfts into a single namelist flag, and then go with something like @slevisconsulting 's first implementation of the fix?

[ekluzek]

Hmm. I was really envisioning to leave the namelist items do_transient_pfts and do_transient_crops in place, but just prevent them from being different in the namelist. The first implementation had the following on a call to a function, which I think is bad coding "do_transient_crops = do_transient_pfts". If we did a clean removal of do_transient_crops, and only use do_transient_pfts, that would be fine. The problem with that in the function call is that looking at it -- you most likely will think it's setting it to itself, and not realize it's changing it on the call. You could add a comment noting this, but I think it's clearer just to not allow the two namelist items to differ.

I think it would be best to have the build-namelist handling ensure they agree with each other for now. In the long run we could remove do_transient_crops as a cleanup step that would be bit-for-bit.

[billsacks]

Sorry, my comment was misleading: I was envisioning something like what you said, where we'd do a clean removal of do_transient_crops. But I'm fine with your plan of doing this in two steps.

[ekluzek]

This is fixed on master, about to move the same fix to release-clm5.0 branch.

[billsacks]

Fixed on master with #540