Why is the CropPhenology call in a doalb conditional?

[billsacks]

Does anyone know why CropPhenology is in a doalb conditional?

https://github.com/escomp/ctsm/blob/master/src/biogeochem/CNPhenologyMod.F90#L358-L363

None of the other phenology calls are in a doalb conditional, so why crop? Looking through the history, it looks like this has been the case ever since the CropPhenology subroutine was introduced in clm4_0_27.

@slevisconsulting @ekluzek @mvertens do any of you remember why we have this doalb conditional here and whether it's important to keep it?

[billsacks]

Also, can anyone comment on when we expect doalb to be true and false? Here is the relevant code, I think:

CTSM/src/cpl/nuopc/lnd_comp_nuopc.F90

Lines 818 to 824 in 8d9f988

	if (nstep == 0) then
	doalb = .false.
	else if (nstep == 1) then
	doalb = (abs(nextsw_cday- caldayp1) < 1.e-10_r8)
	else
	doalb = (nextsw_cday >= -0.5_r8)
	end if

So doalb is always false on time step 0 (i.e., that extra 0th time step we're thinking of getting rid of - #925 ). On time step 1, I'm not sure what to expect in general, though in an I compset that I just ran, doalb is also false in time step 1. For time steps after time step 1, do I understand this code correctly that doalb will always be true for these subsequent time steps?

The main thing I'm interested in at the moment is whether we can rely on CropPhenology always being called in the first time step of each year for all years after the first year of the run, because logic that @samsrabin has added in #1616 relies on this being the case.

[slevis-lmwg]

If my memory serves me, doalb would have been a naïve time saver, because we didn't need to update the phenology more frequently than the albedo. I'm pretty sure that doalb was .true. in an albedo timestep. I cannot comment on whether doalb still serves a useful purpose.

[billsacks]

Thanks for your thoughts, @slevisconsulting . It's also the case that LAIs and SAIs are only updated when doalb is true, so I can see why this doalb conditional around CropPhenology might have been put in to be consistent with that. I guess that begs the question of why LAIs and SAIs are only updated when doalb is true, since LAIs and SAIs influence more than just albedos. (I wonder if there was an intention at some point that phenology should only need to be updated hourly, regardless of the atmosphere's physics time step, e.g., to save time in the reading of satellite phenology, but now there are only remnants of that original intention??? I wonder, for example, if it used to be the case that doalb was false more often in an I compset???)

I think this doalb conditional is going to cause problems for #1616 - see https://github.com/ESCOMP/CTSM/pull/1616/files#r798210796 . So I propose removing this doalb conditional, and replacing it with a conditional that just avoids calling CropPhenology at nstep == 0. I plan to make that change as part of #1623 .

[billsacks]

If there are some situations where doalb is only true on some time steps (e.g., on the hour boundary), then one thing that looks like it could change is the vernalization subroutine: it looks like this is doing some accumulations, so it seems like answers could differ depending on whether it is called (e.g.) every time step vs. every other time step. Again, it seems like doalb is true for every time step after the first two in an I compset, but I'm not sure about coupled configurations.

[billsacks]

Ah, I just found this from the CLM software meeting January 25, 2017:

CropPhenology only called when doalb is true

Even though other phenology-related things don't depend on doalb...

We're not sure why this is here.

Feeling is that this is more likely to cause problems than not, and we should probably remove it.

In terms of testing it: Probably do it in a short simulation...

[samsrabin]

For what it's worth, I tried going back in time with git blame but I hit a wall at the "Move crop branch to trunk" commit (bc3005b) back on 2011-05-02. Maybe some justification was given on a commit message in the crop branch, but I don't know if that's still available anywhere.

[billsacks]

@mvertens says: In a run coupled to CAM, doalb will generally only be true every hour (at other times, nextsw_cday will be negative). I think this gives more motivation for removing the doalb conditional, as I do in #1628 - otherwise the new code in #1616 (specifically, the check of is_beg_curr_year) may not work as intended.

[billsacks]

I feel like I understand this well enough now, so I'm closing this issue. Thanks, all, for your comments.