Merge pull request #329 from xylar/omega/fix-time-step-hh-mm-ss

Add support for time intervals in HH:MM:SS and similar formats

This merge adds support for time intervals in the following formats:
- `DDDD_HH:MM:SS(.sss...)` (previously supported)
- `HH:MM:SS(.sss...)`
- `MM:SS(.sss...)`
- `SS(.sss...)`

This will make compatibility with Polaris and MPAS-Ocean simpler.

Author: xylar

components/omega/doc/devGuide/TimeMgr.md

@@ -135,6 +135,18 @@ Finally, a time interval can be defined as the time between two time instants:
 OMEGA::TimeInterval MyDeltaTime = MyTimeInstant2 - MyTimeInstant1;
 ```
 
+Time intervals can also be created from a formatted string (this is the form
+used when reading configuration options like `TimeStep` and `RunDuration`).
+The supported string formats are:
+
+- `DDDD_HH:MM:SS(.sss...)`
+- `HH:MM:SS(.sss...)`
+- `MM:SS(.sss...)`
+- `SS(.sss...)`
+
+Days, hours and minutes are optional but must be in order if included.
+Fractional seconds are optional.
+
 ### 5. Alarm
 
 The Alarm class is designed to trigger events at specified times. Alarms can be

components/omega/doc/userGuide/Driver.md

@@ -14,8 +14,15 @@ time management of the simulation.
 ```
 The `StartTime` and `StopTime` should be formatted strings in the form
 `YYYY-MM-DD_HH:MM:SS.SSSS` and the `RunDuration` should be a formatted string in
-the form `DDDD_HH:MM:SS.SSSS` (the actual width of each unit can be arbitrary
-and the separators can be any single non-numeric character). Either the
+one of the following forms:
+
+- `DDDD_HH:MM:SS(.sss...)`
+- `HH:MM:SS(.sss...)`
+- `MM:SS(.sss...)`
+- `SS(.sss...)`
+
+The actual width of each unit can be arbitrary and the separators can be any
+single non-numeric character. Either the
 `StopTime` or `RunDuration` can be set to `none` in order to use the other to
 determine the duration of the run. If both are set and `StopTime - StartTime`
 is incosistent with `RunDuration`, then `RunDuration` is used for the

components/omega/doc/userGuide/TimeStepping.md

@@ -32,8 +32,16 @@ The following time steppers are currently available:
 | RungeKutta4 | classic fourth-order four-stage Runge Kutta method |
 
 The time step refers to the main model time step used to advance the solution
-forward. The format is in ``dddd_hh:mm:ss`` for days, hours, minutes and
-seconds.
+forward. The time step is specified as a formatted string and can be provided
+in any of the following forms:
+
+- ``DDDD_HH:MM:SS(.sss...)``
+- ``HH:MM:SS(.sss...)``
+- ``MM:SS(.sss...)``
+- ``SS(.sss...)``
+
+Days, hours and minutes are optional but must be in order if included.
+Fractional seconds are optional.
 
 The StartTime refers to the starting time for the simulation. It is in the
 format ``yyyy-mm-day_hh:mm:ss`` for year, month, day, hour, minute, second.

components/omega/src/infra/TimeMgr.cpp

@@ -32,6 +32,7 @@
 #include "Logging.h"
 
 #include <algorithm>
+#include <cctype>
 #include <cfloat>
 #include <climits>
 #include <cmath>
@@ -2090,7 +2091,8 @@ TimeInterval::TimeInterval(
 // Construct a time interval from a standard string in the form
 // DDDD_HH:MM:SS.SSSS where the width of DD and SS strings can be of
 // arbitrary width (within reason) and the separators can be any single
-// non-numeric character
+// non-numeric character.  DD, HH, MM are optional but must be in order if
+// included.  Fractional seconds are optional.
 TimeInterval::TimeInterval(
     std::string &TimeString // [in] string form of time interval
 ) {
@@ -2099,15 +2101,131 @@ TimeInterval::TimeInterval(
    IsCalendar  = false;
    CalInterval = 0;
 
-   // Extract variables from string
+   // Extract variables from string.
+   // Supported formats:
+   // - DDDD_HH:MM:SS(.sss...)
+   // - HH:MM:SS(.sss...)
+   // - MM:SS(.sss...)
+   // - SS(.sss...)
+   // Separators between fields may be any single non-numeric character.
    I8 Day     = 0;
    I8 Hour    = 0;
    I8 Minute  = 0;
    R8 RSecond = 0.;
 
-   std::istringstream ss(TimeString);
-   char discard;
-   ss >> Day >> discard >> Hour >> discard >> Minute >> discard >> RSecond;
+   // Parse from the right so that '.' is always interpreted as the decimal
+   // point in the seconds field, while still allowing '.' to act as a
+   // separator between integer fields in the legacy format.
+   auto isSpace = [](char c) {
+      return std::isspace(static_cast<unsigned char>(c)) != 0;
+   };
+   auto isDigit = [](char c) {
+      return std::isdigit(static_cast<unsigned char>(c)) != 0;
+   };
+
+   const std::string &s = TimeString;
+   if (s.empty()) {
+      ABORT_ERROR("TimeMgr: empty time interval string");
+   }
+
+   std::size_t right = s.size();
+   while (right > 0 && isSpace(s[right - 1])) {
+      --right;
+   }
+   if (right == 0) {
+      ABORT_ERROR("TimeMgr: blank time interval string");
+   }
+
+   // Parse final seconds token (may include fractional part).
+   std::size_t secEnd = right;
+   std::size_t secBeg = secEnd;
+   while (secBeg > 0) {
+      char c = s[secBeg - 1];
+      if (isDigit(c) || c == '.') {
+         --secBeg;
+      } else if (isSpace(c)) {
+         // allow trailing whitespace only (already trimmed)
+         break;
+      } else {
+         break;
+      }
+   }
+   if (secBeg == secEnd) {
+      ABORT_ERROR("TimeMgr: invalid time interval string '{}'", TimeString);
+   }
+
+   try {
+      RSecond = std::stod(s.substr(secBeg, secEnd - secBeg));
+   } catch (...) {
+      ABORT_ERROR("TimeMgr: invalid seconds field in time interval string '{}'",
+                  TimeString);
+   }
+
+   // Walk left parsing up to 3 integer fields (minute, hour, day).
+   std::size_t idx   = secBeg;
+   auto parsePrevInt = [&](I8 &outVal) -> bool {
+      // Skip whitespace.
+      while (idx > 0 && isSpace(s[idx - 1])) {
+         --idx;
+      }
+      if (idx == 0) {
+         return false;
+      }
+
+      // Skip one or more non-digit separator characters.
+      bool sawSep = false;
+      while (idx > 0 && !isDigit(s[idx - 1]) && !isSpace(s[idx - 1])) {
+         sawSep = true;
+         --idx;
+      }
+      while (idx > 0 && isSpace(s[idx - 1])) {
+         --idx;
+      }
+      if (!sawSep) {
+         return false;
+      }
+      if (idx == 0 || !isDigit(s[idx - 1])) {
+         ABORT_ERROR("TimeMgr: invalid time interval string '{}'", TimeString);
+      }
+
+      std::size_t end = idx;
+      std::size_t beg = end;
+      while (beg > 0 && isDigit(s[beg - 1])) {
+         --beg;
+      }
+      try {
+         outVal = static_cast<I8>(std::stoll(s.substr(beg, end - beg)));
+      } catch (...) {
+         ABORT_ERROR(
+             "TimeMgr: invalid integer field in time interval string '{}'",
+             TimeString);
+      }
+      idx = beg;
+      return true;
+   };
+
+   I8 tmp    = 0;
+   int nInts = 0;
+   if (parsePrevInt(tmp)) {
+      Minute = tmp;
+      ++nInts;
+   }
+   if (parsePrevInt(tmp)) {
+      Hour = tmp;
+      ++nInts;
+   }
+   if (parsePrevInt(tmp)) {
+      Day = tmp;
+      ++nInts;
+   }
+
+   // Anything left besides whitespace is invalid.
+   while (idx > 0 && isSpace(s[idx - 1])) {
+      --idx;
+   }
+   if (idx != 0) {
+      ABORT_ERROR("TimeMgr: invalid time interval string '{}'", TimeString);
+   }
 
    R8 SecondsSet = Day * SECONDS_PER_DAY + Hour * SECONDS_PER_HOUR +
                    Minute * SECONDS_PER_MINUTE + RSecond;

components/omega/test/CMakeLists.txt

@@ -368,6 +368,24 @@ add_omega_test(
     "-n;1"
 )
 
+############################
+# TimeInterval parsing tests
+############################
+
+add_omega_test(
+  TIMEINTERVAL_PARSE_TEST
+  testTimeIntervalParse.exe
+  infra/TimeIntervalParseTest.cpp
+  "-n;1"
+)
+
+add_omega_test(
+  TIMEINTERVAL_PARSE_EXTENDED_FORMATS_TEST
+  testTimeIntervalParseExtendedFormats.exe
+  infra/TimeIntervalParseExtendedFormatsTest.cpp
+  "-n;1"
+)
+
 ##################
 # Reductions test
 ##################

components/omega/test/infra/TimeIntervalParseExtendedFormatsTest.cpp

@@ -0,0 +1,104 @@
+//===-- Test driver for OMEGA TimeInterval extended string formats -*- C++
+//-*-===/
+//
+// This test encodes the *desired* behavior for parsing time interval strings
+// from config (e.g., TimeIntegration::TimeStep and RunDuration).
+//
+// It intentionally checks "short" forms like HH:MM:SS(.sss), MM:SS(.sss), and
+// SS(.sss). Today, the implementation assumes the string always begins with
+// days (DDDD_HH:MM:SS(.sss)), so these cases reproduce the bug.
+//
+// This test is expected to fail until parsing is improved.
+//
+//===----------------------------------------------------------------------===/
+
+#include "DataTypes.h"
+#include "Error.h"
+#include "Logging.h"
+#include "MachEnv.h"
+#include "TimeMgr.h"
+#include "mpi.h"
+
+#include <cmath>
+#include <string>
+
+using namespace OMEGA;
+
+namespace {
+
+struct ExpectedParts {
+   I8 days{0};
+   I8 hours{0};
+   I8 minutes{0};
+   I8 secondsWhole{0};
+   R8 secondsFrac{0.0};
+};
+
+bool nearlyEqual(R8 a, R8 b, R8 tol) { return std::fabs(a - b) <= tol; }
+
+int checkSeconds(const std::string &label, const std::string &input,
+                 const ExpectedParts &exp, R8 tol = 1e-12) {
+
+   const R8 expectedSeconds =
+       static_cast<R8>(exp.days) * static_cast<R8>(SECONDS_PER_DAY) +
+       static_cast<R8>(exp.hours) * static_cast<R8>(SECONDS_PER_HOUR) +
+       static_cast<R8>(exp.minutes) * static_cast<R8>(SECONDS_PER_MINUTE) +
+       static_cast<R8>(exp.secondsWhole) + exp.secondsFrac;
+
+   std::string s = input; // ctor takes non-const ref
+   TimeInterval interval(s);
+
+   R8 seconds{0.0};
+   interval.get(seconds, TimeUnits::Seconds);
+
+   if (!nearlyEqual(seconds, expectedSeconds, tol)) {
+      LOG_ERROR("{}: '{}' parsed seconds mismatch: got {}, expected {}", label,
+                input, seconds, expectedSeconds);
+      return 1;
+   }
+
+   return 0;
+}
+
+} // namespace
+
+int main(int argc, char **argv) {
+
+   MPI_Init(&argc, &argv);
+
+   MachEnv::init(MPI_COMM_WORLD);
+   MachEnv *defEnv = MachEnv::getDefault();
+   initLogging(defEnv);
+
+   int errCount = 0;
+
+   // Desired: HH:MM:SS(.sss)
+   errCount += checkSeconds("Extended TimeInterval parse", "01:23:45",
+                            ExpectedParts{0, 1, 23, 45, 0.0});
+   errCount += checkSeconds("Extended TimeInterval parse", "01:23:45.678",
+                            ExpectedParts{0, 1, 23, 45, 0.678});
+
+   // Desired: MM:SS(.sss)
+   errCount += checkSeconds("Extended TimeInterval parse", "23:45.678",
+                            ExpectedParts{0, 0, 23, 45, 0.678});
+
+   // Desired: SS(.sss)
+   errCount += checkSeconds("Extended TimeInterval parse", "45.678",
+                            ExpectedParts{0, 0, 0, 45, 0.678});
+   errCount += checkSeconds("Extended TimeInterval parse", "45.6",
+                            ExpectedParts{0, 0, 0, 45, 0.6});
+   errCount += checkSeconds("Extended TimeInterval parse", "45.000001",
+                            ExpectedParts{0, 0, 0, 45, 0.000001});
+
+   if (errCount != 0) {
+      LOG_ERROR("TimeIntervalParseExtendedFormatsTest: FAIL ({} errors)",
+                errCount);
+      MPI_Finalize();
+      return 1;
+   }
+
+   LOG_INFO("TimeIntervalParseExtendedFormatsTest: PASS");
+
+   MPI_Finalize();
+   return 0;
+}