Bug in shr_cal_advDate when secIn is not 0

[billsacks]

I'm adding some unit tests of shr_cal_mod as part of #2145 . In doing so, I came across a bug in shr_cal_advDate (the version that accepts real-valued secIN). Here is a unit test that shows the error (I'll be pushing this and some other unit tests to the branch in #2145 soon):

  @Test
  subroutine advDate_int_1dayPlus1sec(this)
    class(TestShrCal), intent(inout) :: this
    integer(i4) :: date_in, date_out
    real(r8) :: sec_in, sec_out

    date_in = 98760317
    sec_in = 100._r8
    call shr_cal_advDate(delta = 86401._r8, units = 'seconds', &
         dateIN = date_in, secIN = sec_in, &
         dateOUT = date_out, secOUT = sec_out, &
         calendar = 'noleap')

    @assertEqual(98760318, date_out)
    @assertEqual(101._r8, sec_out, tolerance=tol)
  end subroutine advDate_int_1dayPlus1sec

The above code shows the expected output. The actual result is date_out=98760317, sec_out=86301.

This change fixes this test (along with a similar test that uses a negative increment of -86401):

diff --git a/src/share/util/shr_cal_mod.F90 b/src/share/util/shr_cal_mod.F90
index eb8297d6a..b84f22140 100644
--- a/src/share/util/shr_cal_mod.F90
+++ b/src/share/util/shr_cal_mod.F90
@@ -786,7 +786,7 @@ subroutine shr_cal_advDate_int(delta,units,dateIN,secIN,dateOUT,secOUT,calendar)
    endif

    ! take secIn into account here since it's real
-   dSec = dSec - secIn
+   dSec = dSec + secIn

    ! i8 math, convert reals to nearest second
    i8dSec = nint(dSec,SHR_KIND_I8)

Looking back at the csm_share history in subversion, I see that dSec used to be computed as dSec + secIn, but @apcraig changed it to dSec - secIn in this tag (which included a major refactor of shr_cal_mod):

Originator: tcraig
Date: Feb 19 2012
Model: share
Version: share3_120219
One-line summary: remove use of shr_cal eday methods, replace them (not bit-for-bit)
  merge between calup02_share3_120123 and calup03_share3_120123

M       shr/shr_tInterp_mod.F90
M       shr/shr_stream_mod.F90
M       shr/shr_cal_mod.F90
M       shr/shr_strdata_mod.F90

@apcraig can you review this to confirm that we should go back to dSec = dSec + secIn?

[billsacks]

This routine is called from just one place in cime:

streams/shr_stream_mod.F90=1469=subroutine shr_stream_readTCoord(strm,k,rc)
streams/shr_stream_mod.F90:1551:      call shr_cal_advDate(tvar(n),bunits,bdate,bsec,ndate,nsec,calendar)

and I don't see any other calls from CESM. I'm not sure about ACME (@rljacob ?) There are more calls to the version that accepts an integer seconds, but that one seems to be working right.

Can anyone suggest a system test that would exercise this code, so that we can either (a) see what impact the bug has, or (b) confirm that the bug has no impact in standard system runs? (@apcraig @mvertens @fischer-ncar ?)

[billsacks]

Note to self: once #2145 comes to master, I'll want to:

(1) Fix the bug in both the int and long versions:

diff --git a/src/share/util/shr_cal_mod.F90 b/src/share/util/shr_cal_mod.F90
index eb8297d6a..29e03e0c1 100644
--- a/src/share/util/shr_cal_mod.F90
+++ b/src/share/util/shr_cal_mod.F90
@@ -786,7 +786,7 @@ subroutine shr_cal_advDate_int(delta,units,dateIN,secIN,dateOUT,secOUT,calendar)
    endif

    ! take secIn into account here since it's real
-   dSec = dSec - secIn
+   dSec = dSec + secIn

    ! i8 math, convert reals to nearest second
    i8dSec = nint(dSec,SHR_KIND_I8)
@@ -873,7 +873,7 @@ subroutine shr_cal_advDate_long(delta,units,dateIN,secIN,dateOUT,secOUT,calendar
    endif

    ! take secIn into account here since it's real
-   dSec = dSec - secIn
+   dSec = dSec + secIn

    ! i8 math, convert reals to nearest second
    i8dSec = nint(dSec,SHR_KIND_I8)

(2) Bring back the failing unit tests, which should now pass (by uncommenting the @Test lines)

(3) Delete the temporary unit tests that have sec_in = 0 (tests with secIn0 in their name)

[billsacks]

This is a duplicate of #858 . However, I'll leave this open until I fix it, because it contains some extra information beyond what's in #858 .