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

irrigate in 1850 is off for runs with use_crop but on for those without #509

Closed
billsacks opened this issue Sep 12, 2018 · 9 comments · Fixed by #1535
Closed

irrigate in 1850 is off for runs with use_crop but on for those without #509

billsacks opened this issue Sep 12, 2018 · 9 comments · Fixed by #1535
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
Milestone

Comments

@billsacks
Copy link
Member

Brief summary of bug

Irrigation seems to be turned on for all non-crop runs, including year-1850 runs. I would expect irrigation to be off in non-crop year-1850 runs, just like it's off in year-1850 runs with use_crop.

General bug information

CTSM version you are using: ctsm1.0.dev011

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

Configurations affected: Non-crop year-1850 runs

Details of bug

Irrigation is turned off for crops in year-1850. I would expect the same to be true for non-crop runs. Instead, it appears that irrigation is turned on in all non-crop runs, even for year 1850. This means that the generic crop is split into irrigated vs. unirrigated in these runs.

It's possible that this is intentional, but this seemed weird to @ekluzek and me.

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

I noticed this by looking through various tests in the aux_clm test suite.

Important output or errors that show the problem

I checked user_nl_clm and, for one test (ERP_Ld5.f10_f10_musgs.I1850Clm50Bgc.cheyenne_gnu.clm-default), confirmed that PCT_CFT indicates that there is truly non-zero area of the irrigated generic crop.

@billsacks
Copy link
Member Author

@dlawrenncar @olyson do you know if this is intentional?

@ekluzek
Copy link
Collaborator

ekluzek commented Sep 12, 2018

An important detail here is that the SP datasets all include irrigated generic crop. So if you have it split out into irrigated generic crop -- can you really turn irrigation off? I suppose setting "irrigate=.false." in this case just means that the irrigated and rainfed CFT's will be separate, but act the same way. So I guess that's OK, so we could turn it off.

I think we want to assess what this should look like though.

@billsacks
Copy link
Member Author

billsacks commented Sep 12, 2018

I'm struggling to see why SP vs. BGC should matter here, since non-crop runs use the same subgrid structure whether they're using SP or BGC (but that may be my lack of understanding).

I do notice that, currently, the collapsing is only done when use_crop is true:

https://github.com/ESCOMP/ctsm/blob/a0ca27c1f67c2d544cfc490a23986cf35247d393/src/main/surfrdMod.F90#L788-L790

I think that, if we can assume that create_crop_landunit is always true (which is the case now, right?) then that should always be called - otherwise, setting irrigate to false won't accomplish anything.

But my understanding here is less than complete, so it's possible that I'm misspeaking....

@olyson
Copy link
Contributor

olyson commented Sep 12, 2018

I would think that irrigation should be off for non-crop 1850 runs as well. I don't think it being on is intentional.

@ekluzek
Copy link
Collaborator

ekluzek commented Sep 21, 2018

From looking at the code (inside of collapse_crop_types), I don't think this will work right now. And I'm pretty sure that's how it ended up the way it is. It should work on the branch that @slevisconsulting is working on in PR #483. So we can change it after that comes to master.

I'm planning on doing a simple test for this when I get a chance to document how it fails. So I'm not 100% sure this is the case, but I'm 95% sure, and we can easily test it.

@billsacks billsacks changed the title irrigate is true for non-crop 1850 runs irrigate in 1850 is off for runs with use_crop but on for those without Sep 24, 2018
@billsacks
Copy link
Member Author

@dlawrenncar feels that, really, we should have irrigation on in 1850 for all configurations. @ekluzek , @dlawrenncar and I can't remember why we decided to turn it off in 1850, but we might have notes somewhere on it. It might have just been a performance issue.

For CESM2.1, we're going to leave this as is. For beyond that, we'll probably change this so that irrigation is always on.

@billsacks
Copy link
Member Author

From a quick search of past meeting notes, the closest I found to a rationale for having irrigation off in 1850 was a reference to this old bugzilla issue: http://bugs.cgd.ucar.edu/show_bug.cgi?id=2476

Dave's comment there was:

Just about everything except the 1850 simulations should have irrigation on as default. I will continue to work to get the message out about the cost that running with irrigation incurs.

which suggests that performance was the main reason for this, though it's not 100% clear.

@billsacks
Copy link
Member Author

Ah, I just found this bugzilla issue which is very similar to the current issue: http://bugs.cgd.ucar.edu/show_bug.cgi?id=2479

Our conclusion here is consistent with the one there: Let's wait for the ability to collapse crop types from a 78 pft dataset down to one without crops; then it will be easier to make crop and non-crop consistent in this respect.

billsacks added a commit to billsacks/ctsm that referenced this issue Jan 13, 2019
Without this, we can't pick up the new clm45 initial conditions files
unless we set use_init_interp = .true.

This should have been set to true for the previous initial conditions,
too: From checking the global metadata on these files, irrigate was
true in the runs that created these files. (See also
ESCOMP#509 - we are currently setting
irrigate to true when use_crop is false.)
@billsacks billsacks added tag: bug - impacts science bug something is working incorrectly and removed type: bug - impacts science labels May 24, 2019
@billsacks billsacks added the priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations label Dec 9, 2021
@billsacks billsacks added this to the ctsm5.1.0 milestone Dec 9, 2021
@billsacks
Copy link
Member Author

Here are the relevant lines:

<!-- Irrigation default -->
<irrigate use_crop=".true." phys="clm5_1"  use_cndv=".false." sim_year_range="1850-2100">.true.</irrigate>
<irrigate use_crop=".true." phys="clm5_1"  use_cndv=".true."  sim_year_range="1850-2100">.false.</irrigate>
<irrigate use_crop=".true." phys="clm5_0" use_cndv=".false."  sim_year_range="1850-2100">.true.</irrigate>
<irrigate use_crop=".true." phys="clm5_0" use_cndv=".true."   sim_year_range="1850-2100">.false.</irrigate>
<irrigate use_crop=".true." phys="clm4_5"                     sim_year_range="1850-2100">.false.</irrigate>

<irrigate use_crop=".true." >.false.</irrigate>
<irrigate use_crop=".false.">.true.</irrigate>

What we'll do is remove the last line, and remove all use_crop attributes from all lines - so the behavior is the same for use_crop true and false. Note that this will change answers for clm5_0, but we're okay with this.

ekluzek added a commit to olyson/ctsm that referenced this issue Feb 9, 2022
…n on for SSP scenarios for clm5_0 or clm5_1 fixing ESCOMP#509
ekluzek added a commit to olyson/ctsm that referenced this issue Feb 9, 2022
… also check for a file with irrig==TRUE, this is needed for ESCOMP#509
@samsrabin samsrabin added the science Enhancement to or bug impacting science label 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
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants