ESCOMP / ESCOMP/CTSM

Make meaning of "prev" consistent between various clm_time_manager routines

Open
#3,026 2 comments 0 reactions 0 assignees View on GitHub
code health investigation priority: low
Dominant language
Fortran
Stars
352
Forks
361
Avg merge
2d 21h
Merged PRs (30d)
7

Description

Emerging from https://github.com/ESCOMP/CTSM/pull/3017#issuecomment-2744267669:

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`).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.