Make meaning of "prev" consistent between various clm_time_manager routines

[billsacks]

Emerging from #3017 (comment):

Some routines in clm_time_manager.F90 query some value for the prev time – i.e., the time at the start of the time step. There are two different ways this is done:

(1) Some use a -dtime offset to a call: get_prev_calday, get_prev_days_per_year and get_prev_yearfrac

(2) Some use the ESMF routine ESMF_ClockGet with the prevTime argument to get the previous time: get_prev_date and get_prev_time

I dug into the implementation of (2) (in the course of working on #3017 ). It turns out that, in the esmf wrf time manager, the implementation of ESMF_ClockGet's prevTime argument matched the implementation in (1). However, with the real ESMF library, the implementation of prevTime uses the stored value of the previous currTime from before the most recent call to ESMF_ClockAdvance.

I think these two will usually be consistent, and I think they'll always be consistent in CTSM for now, since dtime is constant. So as far as I can tell, there aren't currently any problems. But they can theoretically differ – for example, if there is a variable clock time step, or if the clock has its time changed via a set call, rather than only ever changing via an advance of one time step. In any case, though, it's confusing to have these different behaviors, and it could cause subtle problems in the future if there were ever a variable time step, or a jump in time.

To avoid subtle weirdness in the future, and to reduce confusion now, I recommend choosing one or the other of these two. I feel like (1) is more straightforward and less prone to subtle issues under unusual use cases, so I'd recommend changing get_prev_date and get_prev_time to have implementations more like (1) (though note that this will require adding an offset argument to get_curr_time).

[samsrabin]

We talked about this at the SE meeting today, and before noticing you suggested going with (1) we thought (2) would make more sense. This is because we weren't sure if dtime in a variable-timestep context would refer to the time from now to previous or from now to next. What was your thinking, though?

[billsacks]

Possible future weirdness I saw with (2) was:

• If there were a jump in time (maybe for data assimilation purposes), then (2) would give the last value of currTime before the jump, which would be quite different from the start of time at the beginning of the current time step
• Currently, I think advance_timestep is called after initializing the clock, either in a startup or restart run. But if that were ever changed in a refactor, then I think (2) would lead to an undefined or wrong prevTime in the first time step of a run.

This is because we weren't sure if dtime in a variable-timestep context would refer to the time from now to previous or from now to next.

Yes, this also occurred to me. I think that, as long as you wait to update dtime until you're about to advance the time step, then (1) would work correctly. i.e., dtime would refer to the dtime from the last clock advance, not the next clock advance. I can see arguments for doing it either way, but the introduction of a variable time step seems like it will probably require going through all uses of dtime and figuring out which we'd want, and potentially keeping track of both the previous dtime and the upcoming dtime in the time manager - in which case (1) will work as long as the functions are changed to explicitly refer to the previous dtime.