Fix get_curr_yearfrac and get_prev_yearfrac with Gregorian calendar

[billsacks]

Description of changes

Fix get_curr_yearfrac and get_prev_yearfrac with Gregorian calendar.

Also add unit tests of get_prev_yearfrac with a Gregorian calendar.

Specific notes

Contributors other than yourself, if any: @olyson gets most of the credit here

CTSM Issues Fixed (include github issue #):

• Resolves Gridcell water balance errors for some PLUMBER2 sites with branch_tags/PPE.n11_ctsm5.1.dev030 #1590

Are answers expected to change (and if so in what way)? YES, but very limited: From a search through the code, I think this will only impact CNDV cases when run with the Gregorian calendar. (The changed code is also used by the AnnualFluxDribbler, but only to set some values in gridcell-level balance checks; this can cause a run failure as in #1590 but I don't think it will actually cause any answer changes. The changed code is also used for time-interpolated prescribed dynamic landuse (dyn_var_time_interp_type), but we currently don't use that for any variables.) I think (but am not 100% sure) that these changes won't show up in any tests, so I'm marking this PR as bfb, since I think it will be bfb from a testing standpoint.

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

Testing performed, if any: Only unit tests so far

[billsacks]

@olyson feel free to limit your review to the small change in clm_time_manager if you don't want to look through all the unit test changes.

[olyson]

Looks good to me thanks!

[billsacks]

@olyson @ekluzek the commit I just pushed (c432b6d) is NOT yet the fix needed for @olyson 's issue: I first wanted to make the simpler change of removing a similar hack from get_calday, which is used in very limited places in the code and so it seems totally safe to remove the hack from there. The only place this is used is to convert crop planting dates in the format mmdd to a real-valued day-of-year. I did some unit testing (with some temporary tests that I deleted, because they didn't seem valuable long-term) to show that this change won't change answers in this use case, even for a planting date of 1231 (this translates to 366.0 which was left as is by the "hack" code). However, it did raise a different issue that I'll open an issue for shortly.

[billsacks]

@olyson I have a pushed a fix that I expect will solve your latest problem, as we discussed yesterday. Do you want to see if the changes in this branch indeed fix all of your problems now? I have tested the build but nothing more than that yet.

@ekluzek do you want to take a quick look at these changes before I start final testing?

Note that I ended up going with the solution of adding an optional argument to get_curr_calday. This is because I would otherwise have needed to make 8 identical changes to apply the hack to 2 variables in each cap plus 2 more in clm_initializeMod, and that felt like too much duplication.

[ekluzek]

This looks good. The use of that argument "reuse_day_365_for_day_366 " makes the hack put into place more obvious. Hacks like that often cause problems if they aren't more obvious, and that's what happened here. It was OK previously, but because get_curr_calday was assumed to function correctly for all calendars thing built on it could break.

[olyson]

Sorry for the delay. The simulations I'm running only work with the PPE tag so I had to bring in your changes manually as SourceMods. I put in changes to clm_initializeMod.F90, clm_time_manager.F90, and lnd_comp_mct.F90 (I'm not using nuopc or lilac and I figured I didn't need the testing changes).
These did fix the problems I was having, thanks!

[billsacks]

Awesome, thanks @ekluzek and @olyson ! Now I just need to get things working with cime6.0.12 – I'm having problems running the pFUnit unit tests with that cime tag and am looking into why....

[billsacks]

@ekluzek @olyson - I'm working on solving the remaining issue(s) with the Gregorian test. One issue is that there is at least one call to get_days_per_year (in the AnnualFluxDribbler) that really should be looking at the number of days per year in the year corresponding to the start of the time step, not the end of the time step. To encourage callers to think about which one is appropriate for each use, I thought it would be best to make two functions, get_curr_days_per_year and get_prev_days_per_year. I don't love the naming of these functions: I don't feel like the curr and prev are very intuitive in this context, but I thought it would be best to stick with the same naming convention that we use for similar pairs of functions like get_curr_yearfrac vs. get_prev_yearfrac – where curr means "as of the end of the time step" and prev means "as of the start of the time step" (and I have tried to add documentation in the functions to make it clear how they differ). However, please let me know if you feel like these names are too confusing and you have better suggestions.

I'm planning to go through and see if there are other uses of get_days_per_year (i.e., now get_curr_days_per_year) that should actually be using get_prev_days_per_year. My plan is that, for cases where it's not entirely clear what is right, I'll leave it as is, but I want to see if there are other situations where we should really be using get_prev_days_per_year.

[ekluzek]

I agree that you should use curr and prev, since that's what's currently being done. If we change the name for it, we should change it throughout clm_time_manager. And I think it's reasonably clear, so I don't know that we really need to do that wholesale change. But, adding the new function for prev does sound necessary to make this more explicit. It's especially more explicit than the hidden hack that was done previously. So I support your description of what you are doing. Thanks for working through this tricky and subtle aspect of the code!

[olyson]

I also agree that although it's not ideal, we should use curr and prev for consistency. I think your comments will make it clear what the differences are. And again, thanks for your thorough examination of this!

[billsacks]

With the latest changes on this branch, the test SMS_Ly5_Mmpi-serial.1x1_smallvilleIA.IHistClm50BgcCropQianRs.cheyenne_gnu.clm-gregorian_cropMonthOutput now passes. In addition to the issues found by @olyson, I also had to fix the call to get_days_per_year in AnnualFluxDribbler so that it looked at the year as of the start of the time step, not the end of the time step.

Suspecting that there might be other, similar issues in other calls to get_days_per_year, I have looked through all of the calls to this function to see what else should possibly be changed. Here are my notes on the various calls:

• Things I have changed
  • AnnualFluxDribbler: This one should be changed (done)
• Things for which we should probably be using a fixed constant, because the point is to convert a per-year parameter into a per-second value. I'm thinking we should have a function like get_average_days_per_year, which will return 365 for a NOLEAP calendar and 365.2425 for a GREGORIAN calendar.
  • C14Decay: This should probably use a fixed constant, converting the decay rate from a per-year value to a per-second value: as currently written, C14 decays more slowly on leap years.
  • CNGapMortality: This should probably use a fixed constant, converting the mortality rate parameter from a per-year value to a per-second value: as currently written, mortality is a bit slower on leap years.
  • CNPhenologyMod.F90: CNEvergreenPhenology and CNStressDecidPhenology and the multiplier of leaf_long in CropPhenology: This should probably use a fixed constant, converting a litterfall rate from a per-year value to a per-second value: as currently written, litterfall happens more slowly on leap years.
  • decomp_rate_constants_bgc: As for the others, this should probably use a fixed constant
  • ndep_interp: This one is questionable, and may depend in part on whether the ndep input file and the model are consistent in their use of a GREGORIAN vs. NOLEAP calendar. But since I think this will typically come from model run with a NOLEAP calendar, I'm thinking that it should use a fixed constant (so that a constant annual ndep rate on the file would result in a constant annual ndep rate in CTSM, rather than a rate that varies slightly in leap vs. non-leap years). (But from spot-checking some recent files, it looks like rates are typically already specified as per-second, so this conversion from a per-year rate to a per-second rate is never done. I think that this implies that, in a GREGORIAN run, there is slightly more ndep than what occurred in the run that created the forcing data, since a year is slightly longer.)
    • Actually, upon further reflection, I think it's best not to change this one.
• Things I'm not sure about:
  • CNAnnualUpdateMod.F90:CNAnnualUpdate: It feels like this should probably be changed, but I'm not positive. And I don't think it will matter hugely: it looks like problems would just cause this to trigger at slightly the wrong time: For example, in a non-leap year transitioning to a leap year, I have a feeling this might not trigger as intended at the end of Dec 31 (because on the last time step of the year, secspyear will be 366*86400, but the counter will only be 365*86400), but it should trigger at the end of the next Jan 1. And then I think things will work themselves out at the end of the next year to get back in sync.
  • CNFUNInit: I'm not sure what the intent of this code is. It seems like this is supposed to be consistent with some other logic governing when FUN is run, but I'm not sure where that logic is. So I'm not sure what's correct here.
  • CNFireArea and CNFireFluxes (in various CNFire modules): Irrelevant, I think, because it's in a conditional that doesn't get executed on the last day of the year (which is weird and maybe wrong)... if it weren't in that conditional then my intuition is that at least the uses in CNFireFluxes should be get_prev_days_per_year; I'm not sure about the uses in CNFireArea.
  • CNNDynamicnMod.F90: CNFreeLivingFixation and CNNFixation: I'm not sure about these.
  • CNPhenologyMod.F90: CNPhenologyClimate: I'm not sure, but I think it should be consistent with CNAnnualUpdateMod.
  • ch4_annualupdate: I haven't looked carefully at this
  • clmfates_interfaceMod.F90: I haven't looked into this. It's possible that the answer differs for different uses within FATES.
• Other:
  • CropPhenology: determining idpp: see Bugs in crop phenology when running with a Gregorian calendar #1595

So the main changes that I'm pretty sure should be made are those under "Things for which we should probably be using a fixed constant". I am going to open a separate issue for this (update: this is now #1612). I think the changes needed for that will be relatively simple, so I'll take a crack at them this week, but I'd like to do that in a separate tag, partly so those changes can be reviewed separately and partly because they will be more answer-changing than the ones in this PR (for Gregorian cases, at least).

[billsacks]

I did a final pass through the code, looking for any other uses of get_curr_calday, get_curr_yearfrac or get_prev_yearfrac that should be changed. As far as I can tell, everything looks good now; in particular, it looks correct to me not to use reuse_day_365_for_day_366=.true. for any of the other calls.

[billsacks]

Starting final testing now....